From cbb5377307dea33608d51442ccb86e2dec968173 Mon Sep 17 00:00:00 2001 From: Robin Date: Tue, 28 Jul 2026 10:50:38 +0200 Subject: [PATCH 1/5] feat(proxy): re-assert Connect's own login decision after other plugins Connect authenticates tunneled players at its edge and forces offline mode at pre-login. A login plugin that unconditionally forces online mode later in the same event reverts that, and the player then waits forever for an encryption request nothing can answer - the LibreLogin + Connect login hang, and the same for AuthMe, nLogin, or anything else with that behaviour. Connect now registers a second pre-login handler that runs after every other plugin's and restores its own decision. It is a floor, not a veto: it only acts on connections Connect itself authenticated, never overrides another plugin's kick, and does nothing when the decision is still what Connect set. It keys purely off Connect's own result on the event object, so it never reads, links against, or version-checks any third-party plugin - which is what makes it plugin-agnostic and impossible to drift out from under. Ordering, both levers layered: - Velocity registers at short priority Short.MIN_VALUE, one slot below the Short.MIN_VALUE + 1 that PostOrder.LAST maps to, so it wins regardless of load order. That overload only exists on Velocity builds from 2024-09-16 on, so it is feature-detected with one reflective lookup on the public interface; older builds fall back to PostOrder.LAST plus the optional librelogin dependency now declared in velocity-plugin.json, which Velocity turns into load order. velocity-api is deliberately not bumped: reading a PostOrder constant an older runtime lacks throws while Velocity collects the listener's methods and would kill all of Connect's handlers. - BungeeCord uses @EventHandler(priority = Byte.MAX_VALUE); equal priorities there are identity-keyed, so registration order is no lever at all. Two operator controls in proxy config.yml: - login-reassert.enabled (default true) turns the whole thing off, for operators who deliberately want another plugin to override Connect. - login-reassert.restore-full-profile (default false) additionally restores the Mojang UUID and username. Off by default because login plugins key their storage on the proxy UUID; the LibreLogin-side new-uuid-creator: MOJANG prerequisite is documented next to the option. The default restores the skin properties only, which fixes the hang and the missing skin while keeping such plugins internally consistent. VelocityGameProfiles.fromConnectPlayer is now idempotent so the late re-assert cannot append a second copy of Connect's own properties. Guarded by a new :velocity:eventOrderTest source set that runs the real VelocityEventManager and PluginDependencyUtils against a Velocity proxy jar pinned by sha256, plus unit coverage for every fail-safe path. --- AGENTS.md | 27 ++ .../connect/listener/BungeeConnections.java | 64 +++ .../listener/BungeeLateReassertListener.java | 101 +++++ .../connect/listener/BungeeListener.java | 26 +- .../connect/module/BungeeListenerModule.java | 7 + .../connect/BungeePluginStartupTest.java | 2 + .../BungeeLateReassertListenerTest.java | 49 +++ .../connect/config/ProxyConnectConfig.java | 29 ++ core/src/main/resources/proxy-config.yml | 31 ++ .../connect/config/ConfigLoaderTest.java | 73 ++++ docs/login-plugin-integration.md | 83 +++- settings.gradle.kts | 12 + velocity/build.gradle.kts | 30 ++ .../listener/VelocityLateEventOrderTest.java | 375 ++++++++++++++++++ .../listener/VelocityConnectPlayers.java | 63 +++ .../listener/VelocityGameProfiles.java | 30 +- .../listener/VelocityLateEventRegistrar.java | 107 +++++ .../VelocityLateReassertListener.java | 169 ++++++++ .../connect/listener/VelocityListener.java | 17 +- .../VelocityListenerRegistration.java | 6 + .../module/VelocityListenerModule.java | 7 + .../src/main/resources/velocity-plugin.json | 6 + .../connect/VelocityPluginStartupTest.java | 4 + .../listener/VelocityGameProfilesTest.java | 37 ++ .../VelocityLateEventRegistrarTest.java | 107 +++++ .../VelocityLateReassertListenerTest.java | 247 ++++++++++++ 26 files changed, 1666 insertions(+), 43 deletions(-) create mode 100644 bungee/src/main/java/com/minekube/connect/listener/BungeeConnections.java create mode 100644 bungee/src/main/java/com/minekube/connect/listener/BungeeLateReassertListener.java create mode 100644 bungee/src/test/java/com/minekube/connect/listener/BungeeLateReassertListenerTest.java create mode 100644 velocity/src/eventOrderTest/java/com/minekube/connect/listener/VelocityLateEventOrderTest.java create mode 100644 velocity/src/main/java/com/minekube/connect/listener/VelocityConnectPlayers.java create mode 100644 velocity/src/main/java/com/minekube/connect/listener/VelocityLateEventRegistrar.java create mode 100644 velocity/src/main/java/com/minekube/connect/listener/VelocityLateReassertListener.java create mode 100644 velocity/src/test/java/com/minekube/connect/listener/VelocityLateEventRegistrarTest.java create mode 100644 velocity/src/test/java/com/minekube/connect/listener/VelocityLateReassertListenerTest.java diff --git a/AGENTS.md b/AGENTS.md index 2d18afb95..80d181c60 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -80,6 +80,33 @@ curl -I -L --fail https://github.com/minekube/connect-java/releases/download/{@link EventHandler#priority()} is a plain {@code byte} and BungeeCord's event bus + * dispatches the whole byte range, so {@link Byte#MAX_VALUE} runs strictly after + * {@code EventPriority.HIGHEST} (64). That is the only correct lever here: BungeeCord breaks + * ties between equal priorities with a {@code HashMap} keyed by listener identity, so + * registration order - and therefore any {@code softDepends} - decides nothing. + * + *

Same guarantees as on Velocity: only connections Connect itself authenticated are touched, + * another plugin's kick is never overridden, and {@code login-reassert.enabled: false} turns + * the whole thing off. + * + *

BungeeCord's {@code PendingConnection} exposes no profile-properties API at pre-login, so + * the profile half of this is limited to the UUID and username, behind the same opt-in + * {@code login-reassert.restore-full-profile}. + */ +public final class BungeeLateReassertListener implements Listener { + @Inject private ProxyConnectConfig config; + @Inject private ConnectLogger logger; + + @EventHandler(priority = Byte.MAX_VALUE) + public void onPreLoginLate(PreLoginEvent event) { + if (!config.getLoginReassert().isEnabled()) { + return; + } + if (event.isCancelled()) { + return; // a plugin denied this login; never convert a kick into a join + } + try { + PendingConnection connection = event.getConnection(); + LocalSession.context(BungeeConnections.channel(connection), + ctx -> reassert(connection, ctx.getPlayer())); + } catch (Exception exception) { + // Never let the defensive floor itself break a login: without it the connection is + // exactly where it would have been before this listener existed. + logger.error("Failed to re-assert Connect's pre-login decision", exception); + } + } + + private void reassert(PendingConnection connection, ConnectPlayer player) { + if (player.getAuth().isPassthrough()) { + return; // not authenticated by Connect - none of our business + } + if (connection.isOnlineMode()) { + connection.setOnlineMode(false); + logger.debug("Re-asserted offline mode for Connect session {} at pre-login; another " + + "plugin had changed it (set login-reassert.enabled to false to allow that)", + player.getUsername()); + } + if (!config.getLoginReassert().isRestoreFullProfile()) { + return; + } + if (!player.getUniqueId().equals(connection.getUniqueId())) { + connection.setUniqueId(player.getUniqueId()); + } + if (!player.getUsername().equals(connection.getName())) { + BungeeConnections.setName(connection, player.getUsername()); + } + } +} diff --git a/bungee/src/main/java/com/minekube/connect/listener/BungeeListener.java b/bungee/src/main/java/com/minekube/connect/listener/BungeeListener.java index a31ea9b30..186808c7f 100644 --- a/bungee/src/main/java/com/minekube/connect/listener/BungeeListener.java +++ b/bungee/src/main/java/com/minekube/connect/listener/BungeeListener.java @@ -25,8 +25,6 @@ package com.minekube.connect.listener; -import static com.google.common.base.Preconditions.checkNotNull; - import com.google.inject.Inject; import com.minekube.connect.api.ProxyConnectApi; import com.minekube.connect.api.logger.ConnectLogger; @@ -35,34 +33,17 @@ import com.minekube.connect.bedrock.BedrockIdentityEnforcer.Decision; import com.minekube.connect.network.netty.LocalSession; import com.minekube.connect.util.LanguageManager; -import com.minekube.connect.util.ReflectionUtils; -import io.netty.channel.Channel; -import java.lang.reflect.Field; import java.util.UUID; import net.md_5.bungee.api.connection.PendingConnection; import net.md_5.bungee.api.event.LoginEvent; import net.md_5.bungee.api.event.PlayerDisconnectEvent; import net.md_5.bungee.api.event.PreLoginEvent; import net.md_5.bungee.api.plugin.Listener; -import net.md_5.bungee.connection.InitialHandler; import net.md_5.bungee.event.EventHandler; import net.md_5.bungee.event.EventPriority; -import net.md_5.bungee.netty.ChannelWrapper; @SuppressWarnings("ConstantConditions") public final class BungeeListener implements Listener { - private static final Field CHANNEL_WRAPPER; - private static final Field PLAYER_NAME; - - static { - CHANNEL_WRAPPER = - ReflectionUtils.getFieldOfType(InitialHandler.class, ChannelWrapper.class); - checkNotNull(CHANNEL_WRAPPER, "ChannelWrapper field cannot be null"); - - PLAYER_NAME = ReflectionUtils.getField(InitialHandler.class, "name"); - checkNotNull(PLAYER_NAME, "Initial name field cannot be null"); - } - @Inject private ProxyConnectApi api; @Inject private LanguageManager languageManager; @Inject private ConnectLogger logger; @@ -77,10 +58,7 @@ public void onPreLogin(PreLoginEvent event) { PendingConnection connection = event.getConnection(); - ChannelWrapper wrapper = ReflectionUtils.getCastedValue(connection, CHANNEL_WRAPPER); - Channel channel = wrapper.getHandle(); - - LocalSession.context(channel, ctx -> { + LocalSession.context(BungeeConnections.channel(connection), ctx -> { Decision decision = bedrockIdentityEnforcer.verify(ctx); if (!decision.allowed()) { bedrockIdentityEnforcer.reject(ctx, decision); @@ -90,7 +68,7 @@ public void onPreLogin(PreLoginEvent event) { } connection.setOnlineMode(false); connection.setUniqueId(ctx.getPlayer().getUniqueId()); - ReflectionUtils.setValue(connection, PLAYER_NAME, ctx.getPlayer().getUsername()); + BungeeConnections.setName(connection, ctx.getPlayer().getUsername()); // TODO robin: what about profile properties? (but why is skin already showing) }); } diff --git a/bungee/src/main/java/com/minekube/connect/module/BungeeListenerModule.java b/bungee/src/main/java/com/minekube/connect/module/BungeeListenerModule.java index 927d78210..c139f4f1e 100644 --- a/bungee/src/main/java/com/minekube/connect/module/BungeeListenerModule.java +++ b/bungee/src/main/java/com/minekube/connect/module/BungeeListenerModule.java @@ -29,6 +29,7 @@ import com.google.inject.Singleton; import com.google.inject.TypeLiteral; import com.google.inject.multibindings.ProvidesIntoSet; +import com.minekube.connect.listener.BungeeLateReassertListener; import com.minekube.connect.listener.BungeeListener; import com.minekube.connect.register.ListenerRegister; import net.md_5.bungee.api.plugin.Listener; @@ -45,4 +46,10 @@ public Listener bungeeListener() { return new BungeeListener(); } + @Singleton + @ProvidesIntoSet + public Listener bungeeLateReassertListener() { + return new BungeeLateReassertListener(); + } + } diff --git a/bungee/src/test/java/com/minekube/connect/BungeePluginStartupTest.java b/bungee/src/test/java/com/minekube/connect/BungeePluginStartupTest.java index 52545bfb6..04fba203c 100644 --- a/bungee/src/test/java/com/minekube/connect/BungeePluginStartupTest.java +++ b/bungee/src/test/java/com/minekube/connect/BungeePluginStartupTest.java @@ -41,6 +41,7 @@ import com.minekube.connect.bedrock.BedrockIdentityKeyProvider; import com.minekube.connect.inject.CommonPlatformInjector; import com.minekube.connect.inject.bungee.BungeeInjector; +import com.minekube.connect.listener.BungeeLateReassertListener; import com.minekube.connect.listener.BungeeListener; import com.minekube.connect.listener.BungeeListenerRegistration; import com.minekube.connect.module.ProxyCommonModule; @@ -80,6 +81,7 @@ class BungeePluginStartupTest { private static List> bungeeGraphRoots() { List> roots = new ArrayList<>(StartupGraphProvisioning.coreRuntimeGraphRoots()); roots.add(BungeeListener.class); + roots.add(BungeeLateReassertListener.class); roots.add(BungeeCommandUtil.class); roots.add(BungeePlatformUtils.class); roots.add(BungeeInjector.class); diff --git a/bungee/src/test/java/com/minekube/connect/listener/BungeeLateReassertListenerTest.java b/bungee/src/test/java/com/minekube/connect/listener/BungeeLateReassertListenerTest.java new file mode 100644 index 000000000..a5366bb68 --- /dev/null +++ b/bungee/src/test/java/com/minekube/connect/listener/BungeeLateReassertListenerTest.java @@ -0,0 +1,49 @@ +package com.minekube.connect.listener; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.lang.reflect.Method; +import net.md_5.bungee.api.event.PreLoginEvent; +import net.md_5.bungee.event.EventHandler; +import net.md_5.bungee.event.EventPriority; +import org.junit.jupiter.api.Test; + +/** + * Pins BungeeCord's half of the ordering lever. + * + *

{@code EventPriority} is not an enum - it is a set of {@code byte} constants, the highest of + * which is {@code HIGHEST = 64}, while {@code EventHandler#priority()} is a plain {@code byte} + * and BungeeCord's event bus bakes handlers across the entire byte range. So + * {@link Byte#MAX_VALUE} is inside the annotation's declared domain and runs strictly after + * {@code HIGHEST}. + * + *

This is the only correct lever on BungeeCord: handlers of equal priority live in a + * {@code HashMap} keyed by listener identity, so the tie-break is not registration order and no + * {@code softDepends} in {@code plugin.yml} can influence it - unlike on Velocity, where the + * declared dependency graph is a real fallback. + */ +class BungeeLateReassertListenerTest { + @Test + void theReassertRunsAfterEveryOtherPluginsPreLoginHandler() throws Exception { + Method handler = + BungeeLateReassertListener.class.getMethod("onPreLoginLate", PreLoginEvent.class); + + byte priority = handler.getAnnotation(EventHandler.class).priority(); + + assertEquals(Byte.MAX_VALUE, priority, + "the re-assert must run after HIGHEST; nothing lower is a floor"); + assertTrue(priority > EventPriority.HIGHEST); + } + + /** + * The re-assert only adds a floor. Connect's original pre-login handler keeps running first, + * so Connect's ordering relative to every other plugin is otherwise unchanged. + */ + @Test + void theOriginalPreLoginHandlerStillRunsFirst() throws Exception { + Method original = BungeeListener.class.getMethod("onPreLogin", PreLoginEvent.class); + + assertEquals(EventPriority.LOWEST, original.getAnnotation(EventHandler.class).priority()); + } +} diff --git a/core/src/main/java/com/minekube/connect/config/ProxyConnectConfig.java b/core/src/main/java/com/minekube/connect/config/ProxyConnectConfig.java index 6baaeb470..e8fcc698f 100644 --- a/core/src/main/java/com/minekube/connect/config/ProxyConnectConfig.java +++ b/core/src/main/java/com/minekube/connect/config/ProxyConnectConfig.java @@ -32,4 +32,33 @@ */ @Getter public final class ProxyConnectConfig extends ConnectConfig { + private static final LoginReassertConfig DEFAULT_LOGIN_REASSERT = new LoginReassertConfig(); + + /** + * Whether Connect re-asserts its own pre-login decision after every other plugin has run. + */ + private LoginReassertConfig loginReassert = new LoginReassertConfig(); + + /** + * Never {@code null}: a config file written before this option existed keeps the defaults. + */ + public LoginReassertConfig getLoginReassert() { + return loginReassert != null ? loginReassert : DEFAULT_LOGIN_REASSERT; + } + + @Getter + public static class LoginReassertConfig { + /** + * Default on. Turning it off restores the behaviour of leaving Connect's pre-login + * decision to whichever plugin writes last, which is what an operator who deliberately + * wants another plugin to override Connect needs. + */ + private boolean enabled = true; + /** + * Default off. Also restores Connect's UUID and username, not just the skin properties. + * Requires every login plugin on the proxy to key its own storage on the Mojang UUID - + * see the prerequisite documented next to this option in {@code proxy-config.yml}. + */ + private boolean restoreFullProfile = false; + } } diff --git a/core/src/main/resources/proxy-config.yml b/core/src/main/resources/proxy-config.yml index d161c863d..139696480 100644 --- a/core/src/main/resources/proxy-config.yml +++ b/core/src/main/resources/proxy-config.yml @@ -17,6 +17,37 @@ allow-offline-mode-players: false # - pvp-endpoint # - someones-hub +# Connect authenticates tunneled players at its edge and tells the proxy to skip its own +# online-mode handshake for them. Login/auth plugins (LibreLogin, AuthMe, nLogin, ...) that +# force online mode for every connection later in the same login event revert that decision, +# and the player then hangs forever on "Logging in..." because there is no second Mojang +# session to answer the proxy's encryption request. +# +# With this enabled, Connect re-asserts its own decision after every other plugin has run. +# It only ever acts on connections Connect itself authenticated, it never overrides another +# plugin's kick, and it does nothing when nobody changed the decision. +login-reassert: + # Turn this off if you deliberately want another plugin to be able to change Connect's own + # login decision. Off restores the previous behaviour: last writer wins, no floor. + enabled: true + + # Also restore Connect's full game profile - the player's Mojang UUID and username - and + # not just the skin properties. + # + # 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 + # stores players under an offline UUID derived from the name, so restoring the Mojang UUID + # makes its own lookups miss and its join handlers throw on every login. + # + # Leave this off unless you need ConnectApi.isConnectPlayer(uuid) / getPlayer(uuid) to + # resolve for players who go through such a login plugin. The default (skin properties + # only) fixes the login hang and the missing skin while keeping those plugins consistent. + # + # On BungeeCord there is no profile-properties API at pre-login, so `enabled` re-asserts + # offline mode only and this option is what additionally re-asserts the UUID and username. + restore-full-profile: false + # Moxy can attach a signed Bedrock/Xbox identity envelope to Bedrock sessions. # Keep enforcement disabled unless Minekube provided trusted key configuration. bedrock-identity: 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 e9560fe8e..daf5aa9c3 100644 --- a/core/src/test/java/com/minekube/connect/config/ConfigLoaderTest.java +++ b/core/src/test/java/com/minekube/connect/config/ConfigLoaderTest.java @@ -1,6 +1,8 @@ package com.minekube.connect.config; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; import com.minekube.connect.api.logger.ConnectLogger; @@ -71,4 +73,75 @@ void loadsBedrockIdentityEnforcementConfig() throws Exception { assertEquals(120, config.getBedrockIdentity().getMetadataCacheSeconds()); assertEquals("trusted_bedrock_xuid", config.getBedrockIdentity().getExpectedPolicy()); } + + /** + * The login re-assert is on by default and its full-profile half is off by default, including + * for a proxy config written before either key existed. + */ + @Test + void loginReassertDefaultsToOnAndPropertiesOnly() throws Exception { + Files.writeString(tempDir.resolve("config.yml"), String.join("\n", + "endpoint: codexp2p3", + "allow-offline-mode-players: false", + "metrics:", + " disabled: true", + " uuid: 00000000-0000-0000-0000-000000000000", + "config-version: 1", + "")); + + ProxyConnectConfig config = load(ProxyConnectConfig.class); + + assertTrue(config.getLoginReassert().isEnabled(), + "the re-assert must be on unless an operator turns it off"); + assertFalse(config.getLoginReassert().isRestoreFullProfile(), + "restoring the UUID needs an operator-side prerequisite, so it must be opt-in"); + } + + /** Both halves are operator-controllable from the config file. */ + @Test + void loadsLoginReassertOverrides() throws Exception { + Files.writeString(tempDir.resolve("config.yml"), String.join("\n", + "endpoint: codexp2p3", + "allow-offline-mode-players: false", + "login-reassert:", + " enabled: false", + " restore-full-profile: true", + "metrics:", + " disabled: true", + " uuid: 00000000-0000-0000-0000-000000000000", + "config-version: 1", + "")); + + ProxyConnectConfig config = load(ProxyConnectConfig.class); + + assertFalse(config.getLoginReassert().isEnabled()); + assertTrue(config.getLoginReassert().isRestoreFullProfile()); + } + + /** + * The opt-in is only safe with a documented prerequisite; a default with a hidden one is a + * footgun. Pin that the prerequisite is stated where the operator reads the option. + */ + @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); + } + + int optionAt = template.indexOf("restore-full-profile:"); + assertTrue(optionAt > 0, "proxy-config.yml must ship the restore-full-profile option"); + String documentation = template.substring(0, optionAt); + assertTrue(documentation.contains("new-uuid-creator: MOJANG"), + "the LibreLogin-side prerequisite must be documented next to the option"); + } + + private T load(Class configClass) { + return new ConfigLoader( + tempDir, + configClass, + new ConfigLoader.EndpointNameGenerator(new OkHttpClient()), + mock(ConnectLogger.class)).load(); + } } diff --git a/docs/login-plugin-integration.md b/docs/login-plugin-integration.md index 373aabf95..f79f6e7a4 100644 --- a/docs/login-plugin-integration.md +++ b/docs/login-plugin-integration.md @@ -110,6 +110,79 @@ player's UUID, username, game profile, and `Auth`. That is why the attribute val `ConnectPlayer` rather than a bare UUID — presence alone costs you nothing, and the full identity is there when you want it. +## Connect defends its own decision (operators) + +Not every login plugin exempts Connect, so Connect no longer relies on one. On proxies it +registers a second pre-login handler that runs **after every other plugin's** and restores its +own decision if something changed it. That is what makes a LibreLogin/AuthMe/nLogin-style +"force online mode for everyone" stop hanging Connect players at *Logging in…*, with no +upstream change and no need to disable premium autologin. + +It is a **floor, not a veto**: + +- It only ever acts on connections **Connect itself authenticated** (`Auth#isPassthrough()` + is `false`). Everything else is untouched. +- It **never overrides a kick.** If any plugin denied the login, the deny stands — Connect + never turns a kick into a join. +- It does **nothing** when the decision is still what Connect set, which is the normal case + when no conflicting plugin is installed. +- It keys only off *Connect's own* result. Connect does not read, link against, or + version-check any login plugin, so this covers every plugin with this behaviour, present + and future. + +### Config + +Both settings live in Connect's `config.yml` on Velocity and BungeeCord: + +```yaml +login-reassert: + enabled: true + restore-full-profile: false +``` + +**`enabled`** (default `true`) — turn it off if you *deliberately* want another plugin to be +able to change Connect's login decision. Off restores exactly the previous behaviour. + +**`restore-full-profile`** (default `false`) — by default Connect only restores the profile +**properties** (the skin), leaving the UUID as the login plugin set it. Turning this on also +restores the player's **Mojang UUID and username**, which is what makes +`ConnectApi.isConnectPlayer(uuid)` and `getPlayer(uuid)` resolve for these players. + +> **Prerequisite for `restore-full-profile: true`:** every login plugin on the proxy must key +> its own database on the Mojang UUID. For **LibreLogin** that means setting +> **`new-uuid-creator: MOJANG`** in LibreLogin's config *first*. Its default (`CRACKED`) +> stores players under an offline UUID derived from the name, so restoring the Mojang UUID +> makes LibreLogin's own lookups miss and its join handlers throw on every login. This is why +> the option is opt-in rather than the default. + +On BungeeCord there is no profile-properties API at pre-login, so `enabled` re-asserts offline +mode only, and `restore-full-profile` is what additionally re-asserts the UUID and username. + +### How the ordering works (contributors) + +- **Velocity:** the handlers are registered through + `EventManager#register(Object, Class, short, EventHandler)` at `Short.MIN_VALUE`. Velocity + maps `PostOrder.LAST` to `Short.MIN_VALUE + 1`, so this is one slot below it and wins + regardless of plugin load order. That overload only exists on Velocity builds from + 2024-09-16 onwards; it is feature-detected with a single reflective lookup on the public + interface, and older builds fall back to `PostOrder.LAST` plus the `optional` `librelogin` + dependency in `velocity-plugin.json` — Velocity breaks ties between equal orders by plugin + load order, which is a topological sort of the declared dependency graph, and an optional + dependency on an absent plugin is a silent no-op. Velocity's API version is deliberately + **not** bumped: reading a `PostOrder` constant an older runtime lacks throws + `EnumConstantNotPresentException` while Velocity collects the listener's methods, which + would kill *all* of Connect's handlers on older proxies and cannot be guarded against. +- **BungeeCord:** `@EventHandler(priority = Byte.MAX_VALUE)`. `EventPriority` is a set of + `byte` constants (`HIGHEST = 64`), not an enum, and the bus dispatches the whole byte range. + Handlers of equal priority sit in an identity-keyed `HashMap`, so registration order — and + therefore `softDepends` — decides nothing there; the numeric priority is the only lever. + +Pinned by `velocity/src/eventOrderTest/.../VelocityLateEventOrderTest` (which runs the real +`VelocityEventManager` and `PluginDependencyUtils` against a pinned Velocity proxy jar), +`velocity/src/test/.../VelocityLateEventRegistrarTest`, +`velocity/src/test/.../VelocityLateReassertListenerTest` and +`bungee/src/test/.../BungeeLateReassertListenerTest`. + ## Stability commitment **`connect-player` is a permanent public contract.** @@ -135,7 +208,9 @@ build. - **Conflicts with Connect** if it can force online mode at pre-login (`forceOnlineMode()` / `setOnlineMode(true)`), or if it rewrites the game profile after - Connect has set it. + Connect has set it. Connect's own re-assert (above) keeps such a plugin from breaking + logins, but exempting Connect explicitly is still the better fix — it keeps the plugin's own + state consistent instead of having its decision quietly reverted. - **Compatible by design** if it only ever acts on offline-mode connections and never forces online mode. - **If it has a Floodgate exemption**, ask its author to extend that exemption to @@ -150,3 +225,9 @@ build. - `api/src/main/java/com/minekube/connect/api/player/Auth.java` — `isPassthrough()` - `core/src/main/java/com/minekube/connect/network/netty/LocalServerChannelWrapper.java` — the single set-site +- `velocity/src/main/java/com/minekube/connect/listener/VelocityLateReassertListener.java` and + `bungee/src/main/java/com/minekube/connect/listener/BungeeLateReassertListener.java` — the + defensive re-assert +- `velocity/src/main/java/com/minekube/connect/listener/VelocityLateEventRegistrar.java` — the + two layered ordering levers +- `core/src/main/resources/proxy-config.yml` — the `login-reassert` options diff --git a/settings.gradle.kts b/settings.gradle.kts index 1f38f5684..af17065cb 100644 --- a/settings.gradle.kts +++ b/settings.gradle.kts @@ -47,6 +47,18 @@ dependencyResolutionManagement { content { includeGroupByRegex("com\\.github\\..*") } } + // Test-only. PaperMC publishes velocity-api to Maven but not the Velocity *proxy*, and + // :velocity:eventOrderTest asserts Connect's login re-assert against the real + // VelocityEventManager rather than a re-implementation of its ordering rules. The proxy + // jar is served from a content-addressed CDN, so the sha256 in the URL pins the exact + // artifact. Restricted to that single module; nothing else may resolve from here. + ivy("https://fill-data.papermc.io/v1/objects/fb599cbda6a6d01decce5e281f71f51cae7cacffcfafca32a09601f407b0583e/") { + name = "papermc-fill-velocity-proxy" + patternLayout { artifact("[module]-[revision].jar") } + metadataSources { artifact() } + content { includeModule("com.velocitypowered.proxy-jar", "velocity") } + } + } } diff --git a/velocity/build.gradle.kts b/velocity/build.gradle.kts index 8a22001ee..29da2bc68 100644 --- a/velocity/build.gradle.kts +++ b/velocity/build.gradle.kts @@ -29,6 +29,36 @@ tasks.test { useJUnitPlatform() } +// Connect's login re-assert has to run after every other plugin's pre-login handler, which is a +// property of Velocity's dispatcher, not of Connect. So it is asserted against the real +// VelocityEventManager instead of a re-implementation of its ordering rules. +// +// Own source set on purpose: the Velocity *proxy* jar is shaded and carries its own copy of +// velocity-api, which must not shadow the 3.2.0 API the plugin compiles against, nor the +// com.velocitypowered.proxy test stubs the other tests use. The proxy jar is pinned by sha256 - +// see the papermc-fill-velocity-proxy repository in settings.gradle.kts. +val eventOrderTest: SourceSet by sourceSets.creating + +dependencies { + "eventOrderTestImplementation"(sourceSets["main"].output) + "eventOrderTestImplementation"(projects.core) + "eventOrderTestImplementation"("com.velocitypowered.proxy-jar:velocity:3.4.0-566") + "eventOrderTestImplementation"("org.junit.jupiter:junit-jupiter:5.10.5") + "eventOrderTestRuntimeOnly"("org.junit.platform:junit-platform-launcher") +} + +val eventOrderTestTask = tasks.register("eventOrderTest") { + description = "Asserts Connect's login re-assert ordering against a real Velocity proxy." + group = "verification" + testClassesDirs = eventOrderTest.output.classesDirs + classpath = eventOrderTest.runtimeClasspath + useJUnitPlatform() +} + +tasks.check { + dependsOn(eventOrderTestTask) +} + // these dependencies are already present on the platform provided("com.google.code.gson", "gson", gsonVersion) diff --git a/velocity/src/eventOrderTest/java/com/minekube/connect/listener/VelocityLateEventOrderTest.java b/velocity/src/eventOrderTest/java/com/minekube/connect/listener/VelocityLateEventOrderTest.java new file mode 100644 index 000000000..9c6dc447b --- /dev/null +++ b/velocity/src/eventOrderTest/java/com/minekube/connect/listener/VelocityLateEventOrderTest.java @@ -0,0 +1,375 @@ +package com.minekube.connect.listener; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.google.gson.JsonArray; +import com.google.gson.JsonElement; +import com.google.gson.JsonObject; +import com.google.gson.JsonParser; +import com.minekube.connect.api.logger.ConnectLogger; +import com.minekube.connect.api.player.Auth; +import com.minekube.connect.api.player.ConnectPlayer; +import com.minekube.connect.config.ProxyConnectConfig; +import com.minekube.connect.player.ConnectPlayerImpl; +import com.velocitypowered.api.event.PostOrder; +import com.velocitypowered.api.event.Subscribe; +import com.velocitypowered.api.event.connection.PreLoginEvent; +import com.velocitypowered.api.event.connection.PreLoginEvent.PreLoginComponentResult; +import com.velocitypowered.api.network.ProtocolVersion; +import com.velocitypowered.api.plugin.PluginContainer; +import com.velocitypowered.api.plugin.PluginDescription; +import com.velocitypowered.api.plugin.PluginManager; +import com.velocitypowered.api.plugin.meta.PluginDependency; +import com.velocitypowered.api.proxy.InboundConnection; +import com.velocitypowered.proxy.event.VelocityEventManager; +import com.velocitypowered.proxy.plugin.util.PluginDependencyUtils; +import java.io.InputStreamReader; +import java.io.Reader; +import java.net.InetSocketAddress; +import java.lang.reflect.Proxy; +import java.nio.charset.StandardCharsets; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Optional; +import java.util.UUID; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import org.junit.jupiter.api.Test; + +/** + * The ordering half of Connect's defensive login re-assert, asserted against the real + * Velocity proxy - {@link VelocityEventManager} and {@link PluginDependencyUtils} - rather than + * against a re-implementation of their rules. + * + *

What is being defended: Connect authenticates a tunneled player at its edge and forces + * offline mode at {@code PostOrder.EARLY}. A login plugin that unconditionally forces online + * mode at {@code PostOrder.LAST} reverts that, and the player then waits forever for an + * encryption request nothing can answer. The re-assert must therefore land after + * {@code LAST}, which is what {@link VelocityLateEventRegistrar} arranges - and it is a property + * of Velocity's dispatcher, so only Velocity's dispatcher can prove it. + * + *

Runs in its own source set with a real (shaded) Velocity proxy jar on the classpath; see + * {@code velocity/build.gradle.kts}. + */ +class VelocityLateEventOrderTest { + private static final UUID CONNECT_UUID = UUID.fromString("f912bf90-8349-565f-9dc0-9891923c0cc3"); + + /** + * The whole point, end to end: Connect decides offline mode early, a {@code PostOrder.LAST} + * plugin overrides it to online mode, and Connect's re-assert - which must run last - puts it + * back. On the pre-fix code there is no third handler and the login hangs. + */ + @Test + void reassertRunsAfterAPostOrderLastHandlerAndRestoresOfflineMode() throws Exception { + List executed = new ArrayList<>(); + StubPlugin connect = new StubPlugin("connect"); + StubPlugin loginPlugin = new StubPlugin("some-login-plugin"); + VelocityEventManager eventManager = + new VelocityEventManager(new StubPluginManager(connect, loginPlugin)); + + InboundConnection connection = stubConnection(); + VelocityConnectPlayers connectPlayers = new VelocityConnectPlayers(); + + // Connect's existing EARLY handler (VelocityListener#onPreLogin), unchanged by this fix. + eventManager.register(connect, new Object() { + @Subscribe(order = PostOrder.EARLY) + public void onPreLogin(PreLoginEvent event) { + executed.add("connect@EARLY"); + event.setResult(PreLoginComponentResult.forceOfflineMode()); + connectPlayers.remember(event.getConnection(), connectPlayer()); + } + }); + + // Registered BEFORE the login plugin's handler on purpose: Velocity breaks ties between + // equal orders by registration order, so if the re-assert only reached PostOrder.LAST it + // would run first here and lose. Passing this test therefore requires the numeric + // priority below LAST, not merely a late PostOrder. + VelocityLateReassertListener reassert = new VelocityLateReassertListener( + connectPlayers, new ProxyConnectConfig(), new NoopLogger()); + reassert.register(eventManager, connect); + + // Any login plugin that forces online mode for every connection, at the latest order + // @Subscribe can express. + eventManager.register(loginPlugin, new Object() { + @Subscribe(order = PostOrder.LAST) + public void onPreLogin(PreLoginEvent event) { + executed.add("loginPlugin@LAST"); + event.setResult(PreLoginComponentResult.forceOnlineMode()); + } + }); + + PreLoginEvent fired = + eventManager.fire(new PreLoginEvent(connection, "ConnectSteve")).join(); + + assertEquals(Arrays.asList("connect@EARLY", "loginPlugin@LAST"), executed, + "the two pre-existing handlers must still run in their documented order"); + assertTrue(fired.getResult().isForceOfflineMode(), + "Connect's re-assert must be the last writer of its own pre-login decision"); + } + + /** + * A deny from any plugin survives: the re-assert is a floor under Connect's own decision, not + * a veto over anyone else's. + */ + @Test + void aDenyFromALastHandlerIsNeverConvertedIntoAJoin() throws Exception { + StubPlugin connect = new StubPlugin("connect"); + StubPlugin loginPlugin = new StubPlugin("some-login-plugin"); + VelocityEventManager eventManager = + new VelocityEventManager(new StubPluginManager(connect, loginPlugin)); + + InboundConnection connection = stubConnection(); + VelocityConnectPlayers connectPlayers = new VelocityConnectPlayers(); + connectPlayers.remember(connection, connectPlayer()); + + eventManager.register(loginPlugin, new Object() { + @Subscribe(order = PostOrder.LAST) + public void onPreLogin(PreLoginEvent event) { + event.setResult(PreLoginComponentResult.denied( + net.kyori.adventure.text.Component.text("you are banned"))); + } + }); + new VelocityLateReassertListener(connectPlayers, new ProxyConnectConfig(), new NoopLogger()) + .register(eventManager, connect); + + PreLoginEvent fired = + eventManager.fire(new PreLoginEvent(connection, "ConnectSteve")).join(); + + assertTrue(!fired.getResult().isAllowed(), "the kick must survive the re-assert"); + } + + /** + * The numeric-priority lever exists on any Velocity from 2024-09-16 onwards. Connect looks it + * up reflectively on a public interface method; if that lookup ever stops resolving on a + * modern proxy the re-assert silently degrades to a tie at {@code PostOrder.LAST}. + */ + @Test + void modernVelocityExposesTheNumericPriorityRegisterOverload() { + assertNotNull(VelocityLateEventRegistrar.shortRegisterMethod()); + } + + /** + * The fallback lever for Velocity builds older than the overload above: equal orders are + * broken by plugin load order, which is a topological sort of the declared dependency graph. + * Asserted against the descriptor this module actually ships, so deleting the dependency edge + * from {@code velocity-plugin.json} fails here. + */ + @Test + void theShippedDescriptorMakesConnectLoadAfterTheLoginPlugin() throws Exception { + List declared = shippedDependencies(); + assertTrue(declared.stream().allMatch(PluginDependency::isOptional), + "every declared dependency must be optional; a hard one would stop Connect loading"); + assertTrue(declared.stream().anyMatch(d -> d.getId().equals("librelogin")), + "velocity-plugin.json must keep the optional librelogin ordering edge"); + + List withLoginPlugin = sortedIds(Arrays.asList( + description("connect", declared), + description("librelogin", Collections.emptyList()))); + assertEquals(Arrays.asList("librelogin", "connect"), withLoginPlugin, + "the optional dependency must make Connect register its handlers last"); + + // Without the edge Connect sorts first by id and would lose the tie - the reason the edge + // exists at all. + List withoutTheEdge = sortedIds(Arrays.asList( + description("connect", Collections.emptyList()), + description("librelogin", Collections.emptyList()))); + assertEquals(Arrays.asList("connect", "librelogin"), withoutTheEdge); + } + + /** An optional dependency on a plugin that is not installed changes nothing and logs nothing. */ + @Test + void anAbsentOptionalDependencyIsASilentNoOp() throws Exception { + List sorted = sortedIds(Arrays.asList( + description("connect", shippedDependencies()), + description("aaa-unrelated", Collections.emptyList()))); + + assertEquals(Arrays.asList("aaa-unrelated", "connect"), sorted); + } + + private static List sortedIds(List candidates) { + List ids = new ArrayList<>(); + for (PluginDescription description : PluginDependencyUtils.sortCandidates(candidates)) { + ids.add(description.getId()); + } + return ids; + } + + /** The dependencies of the {@code velocity-plugin.json} this module builds into its jar. */ + private static List shippedDependencies() throws Exception { + List dependencies = new ArrayList<>(); + try (Reader reader = new InputStreamReader( + VelocityLateEventOrderTest.class.getResourceAsStream("/velocity-plugin.json"), + StandardCharsets.UTF_8)) { + JsonObject descriptor = JsonParser.parseReader(reader).getAsJsonObject(); + JsonArray declared = descriptor.getAsJsonArray("dependencies"); + assertNotNull(declared, "velocity-plugin.json must declare dependencies"); + for (JsonElement element : declared) { + JsonObject dependency = element.getAsJsonObject(); + dependencies.add(new PluginDependency( + dependency.get("id").getAsString(), + null, + dependency.get("optional").getAsBoolean())); + } + } + return dependencies; + } + + private static PluginDescription description(String id, List dependencies) { + return new PluginDescription() { + @Override + public String getId() { + return id; + } + + @Override + public Collection getDependencies() { + return dependencies; + } + }; + } + + private static ConnectPlayer connectPlayer() { + return new ConnectPlayerImpl( + "session-1", + new com.minekube.connect.api.player.GameProfile( + "ConnectSteve", CONNECT_UUID, Collections.emptyList()), + new Auth(false), + ""); + } + + /** + * A dynamic proxy rather than an implementation: {@link InboundConnection} keeps gaining + * methods across Velocity versions, and this test only needs it as an identity. + */ + private static InboundConnection stubConnection() { + return (InboundConnection) Proxy.newProxyInstance( + VelocityLateEventOrderTest.class.getClassLoader(), + new Class[]{InboundConnection.class}, + (proxy, method, args) -> { + switch (method.getName()) { + case "getRemoteAddress": + return new InetSocketAddress("127.0.0.1", 25565); + case "getVirtualHost": + return Optional.empty(); + case "isActive": + return true; + case "getProtocolVersion": + return ProtocolVersion.MAXIMUM_VERSION; + case "hashCode": + return System.identityHashCode(proxy); + case "equals": + return proxy == args[0]; + case "toString": + return "stub-inbound-connection"; + default: + return null; + } + }); + } + + private static final class StubPlugin implements PluginContainer { + private final PluginDescription description; + private final ExecutorService executor = Executors.newSingleThreadExecutor(); + + StubPlugin(String id) { + this.description = description(id, Collections.emptyList()); + } + + @Override + public PluginDescription getDescription() { + return description; + } + + @Override + public ExecutorService getExecutorService() { + return executor; + } + } + + private static final class StubPluginManager implements PluginManager { + private final List plugins; + + StubPluginManager(StubPlugin... plugins) { + this.plugins = Arrays.asList(plugins); + } + + @Override + public Optional fromInstance(Object instance) { + return plugins.stream() + .filter(plugin -> plugin == instance) + .map(plugin -> (PluginContainer) plugin) + .findFirst(); + } + + @Override + public Optional getPlugin(String id) { + return plugins.stream() + .filter(plugin -> plugin.getDescription().getId().equals(id)) + .map(plugin -> (PluginContainer) plugin) + .findFirst(); + } + + @Override + public Collection getPlugins() { + return new ArrayList<>(plugins); + } + + @Override + public boolean isLoaded(String id) { + return getPlugin(id).isPresent(); + } + + @Override + public void addToClasspath(Object plugin, Path path) { + } + } + + private static final class NoopLogger implements ConnectLogger { + @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) { + } + + @Override + public void trace(String message, Object... args) { + } + + @Override + public void enableDebug() { + } + + @Override + public void disableDebug() { + } + + @Override + public boolean isDebug() { + return false; + } + } +} diff --git a/velocity/src/main/java/com/minekube/connect/listener/VelocityConnectPlayers.java b/velocity/src/main/java/com/minekube/connect/listener/VelocityConnectPlayers.java new file mode 100644 index 000000000..6529614d5 --- /dev/null +++ b/velocity/src/main/java/com/minekube/connect/listener/VelocityConnectPlayers.java @@ -0,0 +1,63 @@ +/* + * Copyright (c) 2021-2022 Minekube. https://minekube.com + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + * + * @author Minekube + * @link https://github.com/minekube/connect-java + */ + +package com.minekube.connect.listener; + +import com.google.common.cache.Cache; +import com.google.common.cache.CacheBuilder; +import com.google.inject.Singleton; +import com.minekube.connect.api.player.ConnectPlayer; +import com.velocitypowered.api.proxy.InboundConnection; +import java.util.concurrent.TimeUnit; + +/** + * The Connect players of the connections currently logging in, shared by every Velocity + * listener so that a handler running late in the login sequence can still tell whether the + * connection was authenticated by Connect. + */ +@Singleton +public final class VelocityConnectPlayers { + private final Cache cache = + CacheBuilder.newBuilder() + .maximumSize(500) + .expireAfterAccess(20, TimeUnit.SECONDS) + .build(); + + /** + * Remembers the Connect player of a connection Connect itself authenticated. + */ + public void remember(InboundConnection connection, ConnectPlayer player) { + cache.put(connection, player); + } + + /** + * Returns the Connect player of this connection, or {@code null} when Connect did not + * authenticate it. Entries expire on their own; nothing removes them explicitly, because a + * handler registered after every other plugin's must still be able to read them. + */ + public ConnectPlayer get(InboundConnection connection) { + return cache.getIfPresent(connection); + } +} diff --git a/velocity/src/main/java/com/minekube/connect/listener/VelocityGameProfiles.java b/velocity/src/main/java/com/minekube/connect/listener/VelocityGameProfiles.java index 7f09a447c..35158101a 100644 --- a/velocity/src/main/java/com/minekube/connect/listener/VelocityGameProfiles.java +++ b/velocity/src/main/java/com/minekube/connect/listener/VelocityGameProfiles.java @@ -6,6 +6,7 @@ import com.velocitypowered.api.util.GameProfile; import com.velocitypowered.api.util.GameProfile.Property; import java.util.List; +import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -13,15 +14,32 @@ final class VelocityGameProfiles { private VelocityGameProfiles() { } + /** + * The profile Connect wants this connection to have: Connect's identity and properties, plus + * whatever else the base profile carried. + * + *

Idempotent on purpose. {@link VelocityLateReassertListener} applies this a second time, + * after every other plugin, on a base that already went through it once - Connect's own + * properties must replace their earlier copies rather than be appended to them. + */ static GameProfile fromConnectPlayer(GameProfile base, ConnectPlayer player) { + List connectProperties = + BedrockIdentityProfiles.withoutEnvelope(player.getGameProfile()).getProperties() + .stream() + .map(property -> new Property( + property.getName(), + property.getValue(), + property.getSignature())) + .collect(Collectors.toList()); + Set connectPropertyNames = connectProperties.stream() + .map(Property::getName) + .collect(Collectors.toSet()); + List properties = Stream.concat( base.getProperties().stream() - .filter(property -> !isPrivateIdentity(property)), - BedrockIdentityProfiles.withoutEnvelope(player.getGameProfile()).getProperties().stream() - .map(property -> new Property( - property.getName(), - property.getValue(), - property.getSignature()))) + .filter(property -> !isPrivateIdentity(property)) + .filter(property -> !connectPropertyNames.contains(property.getName())), + connectProperties.stream()) .collect(Collectors.toList()); return base.withId(player.getUniqueId()) .withName(player.getUsername()) diff --git a/velocity/src/main/java/com/minekube/connect/listener/VelocityLateEventRegistrar.java b/velocity/src/main/java/com/minekube/connect/listener/VelocityLateEventRegistrar.java new file mode 100644 index 000000000..5b366c16a --- /dev/null +++ b/velocity/src/main/java/com/minekube/connect/listener/VelocityLateEventRegistrar.java @@ -0,0 +1,107 @@ +/* + * Copyright (c) 2021-2022 Minekube. https://minekube.com + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + * + * @author Minekube + * @link https://github.com/minekube/connect-java + */ + +package com.minekube.connect.listener; + +import com.velocitypowered.api.event.EventHandler; +import com.velocitypowered.api.event.EventManager; +import com.velocitypowered.api.event.PostOrder; +import java.lang.reflect.Method; + +/** + * Registers event handlers that must run strictly after every other plugin's handler. + * + *

Two layered levers, because neither covers the whole range of Velocity versions Connect + * supports on its own: + * + *

    + *
  1. Numeric priority. Velocity maps {@link PostOrder#LAST} to + * {@code Short.MIN_VALUE + 1}, deliberately leaving exactly one slot below it. + * {@link #AFTER_LAST} takes that slot, so the handler runs after every {@code LAST} + * handler no matter which plugin loaded first. The + * {@code register(Object, Class, short, EventHandler)} overload that reaches it only + * exists on Velocity builds from 2024-09-16 onwards, which is why it is looked up + * reflectively - on a public interface method, so its absence is a plain + * {@link NoSuchMethodException} rather than anything that can break class loading.
    2. + *
  2. Load order. On older builds the handler falls back to {@code PostOrder.LAST}. + * Velocity breaks ties between equal orders by registration order, which is plugin load + * order, which is a topological sort of the declared dependency graph - so the + * {@code optional} plugin dependencies declared in {@code velocity-plugin.json} put + * Connect after them. An optional dependency on a plugin that is not installed is a + * silent no-op.
    3. + *
+ * + *

This class knows nothing about any specific third-party plugin, and it never reflects + * into Velocity internals: both levers are public API. + */ +final class VelocityLateEventRegistrar { + /** + * One slot below {@code PostOrder.LAST}, which Velocity maps to {@code Short.MIN_VALUE + 1}. + * It is also the default of {@code Subscribe#priority()}, so a handler registered here still + * runs after anything a plugin puts at {@code PostOrder.CUSTOM} without picking a priority. + */ + static final short AFTER_LAST = Short.MIN_VALUE; + + private final EventManager eventManager; + private final Object plugin; + + VelocityLateEventRegistrar(EventManager eventManager, Object plugin) { + this.eventManager = eventManager; + this.plugin = plugin; + } + + /** + * The {@code register(Object, Class, short, EventHandler)} overload of the running Velocity's + * {@link EventManager}, or {@code null} on Velocity builds that predate it. + */ + static Method shortRegisterMethod() { + try { + return EventManager.class.getMethod( + "register", Object.class, Class.class, short.class, EventHandler.class); + } catch (ReflectiveOperationException | LinkageError absentOnOlderVelocity) { + return null; + } + } + + /** + * Registers {@code handler} so that it runs after every other handler of {@code eventType}. + * + * @return whether the numeric-priority lever was available; {@code false} means the handler + * was registered at {@link PostOrder#LAST} instead and relies on load order + */ + boolean registerAfterLast(Class eventType, EventHandler handler) { + Method shortRegister = shortRegisterMethod(); + if (shortRegister != null) { + try { + shortRegister.invoke(eventManager, plugin, eventType, AFTER_LAST, handler); + return true; + } catch (ReflectiveOperationException | LinkageError ignored) { + // Fall through to the load-order lever below. + } + } + eventManager.register(plugin, eventType, PostOrder.LAST, handler); + return false; + } +} diff --git a/velocity/src/main/java/com/minekube/connect/listener/VelocityLateReassertListener.java b/velocity/src/main/java/com/minekube/connect/listener/VelocityLateReassertListener.java new file mode 100644 index 000000000..516458a84 --- /dev/null +++ b/velocity/src/main/java/com/minekube/connect/listener/VelocityLateReassertListener.java @@ -0,0 +1,169 @@ +/* + * Copyright (c) 2021-2022 Minekube. https://minekube.com + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + * + * @author Minekube + * @link https://github.com/minekube/connect-java + */ + +package com.minekube.connect.listener; + +import com.google.inject.Inject; +import com.minekube.connect.api.logger.ConnectLogger; +import com.minekube.connect.api.player.ConnectPlayer; +import com.minekube.connect.config.ProxyConnectConfig; +import com.velocitypowered.api.event.EventManager; +import com.velocitypowered.api.event.connection.PreLoginEvent; +import com.velocitypowered.api.event.connection.PreLoginEvent.PreLoginComponentResult; +import com.velocitypowered.api.event.player.GameProfileRequestEvent; +import com.velocitypowered.api.util.GameProfile; +import com.velocitypowered.api.util.GameProfile.Property; +import java.util.List; + +/** + * Re-asserts Connect's own login decision after every other plugin has had its say. + * + *

Connect authenticates a tunneled player at the Minekube edge and hands the proxy an + * already-verified, offline-mode connection ({@link VelocityListener}'s + * {@code PostOrder.EARLY} handlers). A login plugin that unconditionally forces online mode + * later in the same event reverts that, and the proxy then sends an encryption request that + * nothing can answer - the player hangs at "Logging in..." forever. + * + *

This listener is a floor, not a veto. It reacts only to Connect's own result + * having been changed on the event object: + * + *

+ * + *

Because the trigger is Connect's own observed state, this covers every plugin that forces + * online mode after Connect - LibreLogin, AuthMe, nLogin, anything - without Connect reading, + * linking against, or version-checking any of them. + * + *

By default only the profile properties (the skin) are restored, not the UUID: + * login plugins commonly key their own database on the UUID the proxy ends up with, and + * changing it out from under them breaks their lookups. Restoring the full profile is + * available as an opt-in, with its prerequisite documented in {@code proxy-config.yml}. + * + * @see VelocityLateEventRegistrar for how "after every other plugin" is achieved + */ +public final class VelocityLateReassertListener { + @Inject private VelocityConnectPlayers connectPlayers; + @Inject private ProxyConnectConfig config; + @Inject private ConnectLogger logger; + + public VelocityLateReassertListener() { + } + + VelocityLateReassertListener( + VelocityConnectPlayers connectPlayers, + ProxyConnectConfig config, + ConnectLogger logger) { + this.connectPlayers = connectPlayers; + this.config = config; + this.logger = logger; + } + + /** + * Registers both handlers after every other plugin's. Called instead of the usual annotated + * listener registration, because {@code @Subscribe} cannot express "after {@code LAST}" on + * the Velocity API Connect compiles against. + */ + public void register(EventManager eventManager, Object plugin) { + VelocityLateEventRegistrar registrar = new VelocityLateEventRegistrar(eventManager, plugin); + boolean afterLast = registrar.registerAfterLast(PreLoginEvent.class, this::onPreLoginLate); + registrar.registerAfterLast(GameProfileRequestEvent.class, this::onGameProfileRequestLate); + logger.debug( + "Registered the login re-assert handlers (numeric priority: {})", + afterLast ? "yes" : "no, this Velocity only supports PostOrder.LAST"); + } + + void onPreLoginLate(PreLoginEvent event) { + if (!enabled()) { + return; + } + if (connectPlayers.get(event.getConnection()) == null) { + return; // not authenticated by Connect - none of our business + } + PreLoginComponentResult result = event.getResult(); + if (!result.isAllowed()) { + return; // a plugin denied this login; never convert a kick into a join + } + if (result.isForceOfflineMode()) { + return; // still what Connect set + } + event.setResult(PreLoginComponentResult.forceOfflineMode()); + logger.debug("Re-asserted offline mode for a Connect session at pre-login; " + + "another plugin had changed it (set login-reassert.enabled to false to allow that)"); + } + + void onGameProfileRequestLate(GameProfileRequestEvent event) { + if (!enabled() || event.isOnlineMode()) { + return; + } + ConnectPlayer player = connectPlayers.get(event.getConnection()); + if (player == null) { + return; + } + GameProfile current = event.getGameProfile(); + GameProfile connectProfile = VelocityGameProfiles.fromConnectPlayer(current, player); + GameProfile wanted = config.getLoginReassert().isRestoreFullProfile() + ? connectProfile + : current.withProperties(connectProfile.getProperties()); + if (sameProfile(current, wanted)) { + return; + } + event.setGameProfile(wanted); + logger.debug("Re-asserted the game profile of Connect session {}", player.getUsername()); + } + + private boolean enabled() { + return config.getLoginReassert().isEnabled(); + } + + /** + * {@link GameProfile} and its {@link Property} do not implement {@code equals}, so compare + * the parts this listener can change. + */ + private static boolean sameProfile(GameProfile left, GameProfile right) { + if (!left.getId().equals(right.getId()) || !left.getName().equals(right.getName())) { + return false; + } + List leftProperties = left.getProperties(); + List rightProperties = right.getProperties(); + if (leftProperties.size() != rightProperties.size()) { + return false; + } + for (int i = 0; i < leftProperties.size(); i++) { + Property a = leftProperties.get(i); + Property b = rightProperties.get(i); + if (!a.getName().equals(b.getName()) + || !a.getValue().equals(b.getValue()) + || !a.getSignature().equals(b.getSignature())) { + return false; + } + } + return true; + } +} diff --git a/velocity/src/main/java/com/minekube/connect/listener/VelocityListener.java b/velocity/src/main/java/com/minekube/connect/listener/VelocityListener.java index 5580391a0..9ff8726d2 100644 --- a/velocity/src/main/java/com/minekube/connect/listener/VelocityListener.java +++ b/velocity/src/main/java/com/minekube/connect/listener/VelocityListener.java @@ -32,8 +32,6 @@ import static com.minekube.connect.util.ReflectionUtils.getPrefixedClassSilently; import static com.minekube.connect.util.ReflectionUtils.getValue; -import com.google.common.cache.Cache; -import com.google.common.cache.CacheBuilder; import com.google.inject.Inject; import com.minekube.connect.api.ProxyConnectApi; import com.minekube.connect.api.logger.ConnectLogger; @@ -53,7 +51,6 @@ import io.netty.channel.Channel; import java.lang.reflect.Field; import java.util.Objects; -import java.util.concurrent.TimeUnit; import net.kyori.adventure.text.Component; public final class VelocityListener { @@ -82,16 +79,11 @@ public final class VelocityListener { CHANNEL = getFieldOfType(minecraftConnection, Channel.class); } - private final Cache playerCache = - CacheBuilder.newBuilder() - .maximumSize(500) - .expireAfterAccess(20, TimeUnit.SECONDS) - .build(); - @Inject private ProxyConnectApi api; @Inject private LanguageManager languageManager; @Inject private ConnectLogger logger; @Inject private BedrockIdentityEnforcer bedrockIdentityEnforcer; + @Inject private VelocityConnectPlayers connectPlayers; @Subscribe(order = PostOrder.EARLY) public void onPreLogin(PreLoginEvent event) { @@ -117,7 +109,7 @@ public void onPreLogin(PreLoginEvent event) { if (!ctx.getPlayer().getAuth().isPassthrough()) { // Means the TunnelService has already authenticated the player event.setResult(PreLoginEvent.PreLoginComponentResult.forceOfflineMode()); - playerCache.put(event.getConnection(), ctx.getPlayer()); + connectPlayers.remember(event.getConnection(), ctx.getPlayer()); } }); } catch (Exception exception) { @@ -130,9 +122,10 @@ public void onGameProfileRequest(GameProfileRequestEvent event) { if (event.isOnlineMode()) { return; } - ConnectPlayer player = playerCache.getIfPresent(event.getConnection()); + ConnectPlayer player = connectPlayers.get(event.getConnection()); if (player != null) { - playerCache.invalidate(event.getConnection()); + // Deliberately not removed here: VelocityLateReassertListener runs after every + // other plugin's handler and still needs to recognise this connection. // Use the game profile received from WatchService for this connection event.setGameProfile(VelocityGameProfiles.fromConnectPlayer(event.getGameProfile(), player)); } diff --git a/velocity/src/main/java/com/minekube/connect/listener/VelocityListenerRegistration.java b/velocity/src/main/java/com/minekube/connect/listener/VelocityListenerRegistration.java index 74d4877fa..a3037291b 100644 --- a/velocity/src/main/java/com/minekube/connect/listener/VelocityListenerRegistration.java +++ b/velocity/src/main/java/com/minekube/connect/listener/VelocityListenerRegistration.java @@ -37,6 +37,12 @@ public final class VelocityListenerRegistration implements ListenerRegistration< @Override public void register(Object listener) { + if (listener instanceof VelocityLateReassertListener) { + // Cannot be an annotated listener: @Subscribe has no way to express "after + // PostOrder.LAST" on the Velocity API this plugin compiles against. + ((VelocityLateReassertListener) listener).register(eventManager, plugin); + return; + } eventManager.register(plugin, listener); } } diff --git a/velocity/src/main/java/com/minekube/connect/module/VelocityListenerModule.java b/velocity/src/main/java/com/minekube/connect/module/VelocityListenerModule.java index 6ca601446..72cc231a6 100644 --- a/velocity/src/main/java/com/minekube/connect/module/VelocityListenerModule.java +++ b/velocity/src/main/java/com/minekube/connect/module/VelocityListenerModule.java @@ -29,6 +29,7 @@ import com.google.inject.Singleton; import com.google.inject.TypeLiteral; import com.google.inject.multibindings.ProvidesIntoSet; +import com.minekube.connect.listener.VelocityLateReassertListener; import com.minekube.connect.listener.VelocityListener; import com.minekube.connect.register.ListenerRegister; @@ -44,4 +45,10 @@ public Object velocityListener() { return new VelocityListener(); } + @Singleton + @ProvidesIntoSet + public Object velocityLateReassertListener() { + return new VelocityLateReassertListener(); + } + } diff --git a/velocity/src/main/resources/velocity-plugin.json b/velocity/src/main/resources/velocity-plugin.json index b56433167..8fc36225c 100644 --- a/velocity/src/main/resources/velocity-plugin.json +++ b/velocity/src/main/resources/velocity-plugin.json @@ -7,5 +7,11 @@ "authors": [ "${author}" ], + "dependencies": [ + { + "id": "librelogin", + "optional": true + } + ], "main": "com.minekube.connect.VelocityPlugin" } \ No newline at end of file diff --git a/velocity/src/test/java/com/minekube/connect/VelocityPluginStartupTest.java b/velocity/src/test/java/com/minekube/connect/VelocityPluginStartupTest.java index c748aa0c3..5db0cc365 100644 --- a/velocity/src/test/java/com/minekube/connect/VelocityPluginStartupTest.java +++ b/velocity/src/test/java/com/minekube/connect/VelocityPluginStartupTest.java @@ -40,6 +40,8 @@ import com.minekube.connect.bedrock.BedrockIdentityEnforcer; import com.minekube.connect.bedrock.BedrockIdentityKeyProvider; import com.minekube.connect.inject.velocity.VelocityInjector; +import com.minekube.connect.listener.VelocityConnectPlayers; +import com.minekube.connect.listener.VelocityLateReassertListener; import com.minekube.connect.listener.VelocityListener; import com.minekube.connect.listener.VelocityListenerRegistration; import com.minekube.connect.module.ProxyCommonModule; @@ -83,6 +85,8 @@ private static List> velocityGraphRoots() { List> roots = new ArrayList<>(StartupGraphProvisioning.coreRuntimeGraphRoots()); roots.add(VelocityPlugin.class); roots.add(VelocityListener.class); + roots.add(VelocityLateReassertListener.class); + roots.add(VelocityConnectPlayers.class); roots.add(VelocityCommandUtil.class); roots.add(VelocityPlatformUtils.class); roots.add(VelocityInjector.class); diff --git a/velocity/src/test/java/com/minekube/connect/listener/VelocityGameProfilesTest.java b/velocity/src/test/java/com/minekube/connect/listener/VelocityGameProfilesTest.java index 248455376..8b102c246 100644 --- a/velocity/src/test/java/com/minekube/connect/listener/VelocityGameProfilesTest.java +++ b/velocity/src/test/java/com/minekube/connect/listener/VelocityGameProfilesTest.java @@ -11,6 +11,7 @@ import com.velocitypowered.api.util.GameProfile.Property; import java.util.Arrays; import java.util.UUID; +import java.util.stream.Collectors; import org.junit.jupiter.api.Test; class VelocityGameProfilesTest { @@ -62,4 +63,40 @@ void forwardedProfileExcludesIdentityEnvelopeAndNonceFromEverySource() { assertFalse(forwarded.toString().contains("replay-nonce")); assertFalse(forwarded.toString().contains("private-endpoint")); } + + /** + * The late re-assert applies this to a profile it already produced once, so Connect's own + * properties must replace their earlier copies. Appending them would leave the player with + * two {@code textures} properties. + */ + @Test + void reapplyingConnectsProfileReplacesItsOwnPropertiesInsteadOfDuplicatingThem() { + UUID uuid = UUID.fromString("f912bf90-8349-565f-9dc0-9891923c0cc3"); + GameProfile base = new GameProfile( + UUID.randomUUID(), + "Original", + Arrays.asList(new Property("other-plugin", "keep-me", ""))); + ConnectPlayer player = new ConnectPlayerImpl( + "session-1", + new com.minekube.connect.api.player.GameProfile( + "ConnectSteve", + uuid, + Arrays.asList( + new com.minekube.connect.api.player.GameProfile.Property( + "textures", "skin", "signature"))), + new Auth(false), + ""); + + GameProfile once = VelocityGameProfiles.fromConnectPlayer(base, player); + GameProfile twice = VelocityGameProfiles.fromConnectPlayer(once, player); + + assertEquals(1, twice.getProperties().stream() + .filter(property -> "textures".equals(property.getName())) + .count()); + assertEquals( + once.getProperties().stream().map(Property::getName).collect(Collectors.toList()), + twice.getProperties().stream().map(Property::getName).collect(Collectors.toList())); + assertEquals(uuid, twice.getId()); + assertEquals("ConnectSteve", twice.getName()); + } } diff --git a/velocity/src/test/java/com/minekube/connect/listener/VelocityLateEventRegistrarTest.java b/velocity/src/test/java/com/minekube/connect/listener/VelocityLateEventRegistrarTest.java new file mode 100644 index 000000000..974c793a4 --- /dev/null +++ b/velocity/src/test/java/com/minekube/connect/listener/VelocityLateEventRegistrarTest.java @@ -0,0 +1,107 @@ +package com.minekube.connect.listener; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; + +import com.velocitypowered.api.event.EventHandler; +import com.velocitypowered.api.event.EventManager; +import com.velocitypowered.api.event.PostOrder; +import com.velocitypowered.api.event.connection.PreLoginEvent; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import org.junit.jupiter.api.Test; + +/** + * The old-Velocity half of the ordering lever, exercised for real: this module compiles against + * velocity-api 3.2.0-SNAPSHOT, which predates + * {@code EventManager#register(Object, Class, short, EventHandler)}. So the reflective lookup + * genuinely fails here and the fallback path is the one under test — exactly what happens on a + * Velocity build older than 2024-09-16. + * + *

The other half — the numeric-priority path, and that it really does run after + * {@code PostOrder.LAST} — is proven against a real {@code VelocityEventManager} in + * {@code VelocityLateEventOrderTest} ({@code velocity/src/eventOrderTest}). + */ +class VelocityLateEventRegistrarTest { + @Test + void velocityApiWithoutTheShortOverloadFallsBackToPostOrderLast() { + assertNull(VelocityLateEventRegistrar.shortRegisterMethod(), + "velocity-api 3.2.0-SNAPSHOT has no short register overload; if this starts " + + "failing the compile-time API was bumped and the fallback is untested"); + + RecordingEventManager eventManager = new RecordingEventManager(); + Object plugin = new Object(); + EventHandler handler = event -> { }; + + boolean usedNumericPriority = new VelocityLateEventRegistrar(eventManager, plugin) + .registerAfterLast(PreLoginEvent.class, handler); + + assertFalse(usedNumericPriority); + assertEquals(1, eventManager.registrations.size()); + Registration registration = eventManager.registrations.get(0); + assertSame(plugin, registration.plugin); + assertSame(PreLoginEvent.class, registration.eventType); + assertEquals(PostOrder.LAST, registration.postOrder); + assertSame(handler, registration.handler); + } + + /** + * Velocity maps {@code PostOrder.LAST} to {@code Short.MIN_VALUE + 1}, deliberately leaving + * one slot below it. Losing that relationship silently turns the floor into a tie. + */ + @Test + void afterLastSitsOneSlotBelowPostOrderLast() { + assertEquals((short) (Short.MIN_VALUE + 1), + (short) (VelocityLateEventRegistrar.AFTER_LAST + 1)); + } + + private static final class Registration { + final Object plugin; + final Class eventType; + final PostOrder postOrder; + final EventHandler handler; + + Registration(Object plugin, Class eventType, PostOrder postOrder, + EventHandler handler) { + this.plugin = plugin; + this.eventType = eventType; + this.postOrder = postOrder; + this.handler = handler; + } + } + + private static final class RecordingEventManager implements EventManager { + final List registrations = new ArrayList<>(); + + @Override + public void register(Object plugin, Object listener) { + throw new AssertionError("the late re-assert must not register as an annotated listener"); + } + + @Override + public void register(Object plugin, Class eventClass, PostOrder postOrder, + EventHandler handler) { + registrations.add(new Registration(plugin, eventClass, postOrder, handler)); + } + + @Override + public CompletableFuture fire(E event) { + return CompletableFuture.completedFuture(event); + } + + @Override + public void unregisterListeners(Object plugin) { + } + + @Override + public void unregisterListener(Object plugin, Object listener) { + } + + @Override + public void unregister(Object plugin, EventHandler handler) { + } + } +} diff --git a/velocity/src/test/java/com/minekube/connect/listener/VelocityLateReassertListenerTest.java b/velocity/src/test/java/com/minekube/connect/listener/VelocityLateReassertListenerTest.java new file mode 100644 index 000000000..85730daf3 --- /dev/null +++ b/velocity/src/test/java/com/minekube/connect/listener/VelocityLateReassertListenerTest.java @@ -0,0 +1,247 @@ +package com.minekube.connect.listener; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; + +import com.minekube.connect.api.logger.ConnectLogger; +import com.minekube.connect.api.player.Auth; +import com.minekube.connect.api.player.ConnectPlayer; +import com.minekube.connect.config.ProxyConnectConfig; +import com.minekube.connect.config.ProxyConnectConfig.LoginReassertConfig; +import com.minekube.connect.player.ConnectPlayerImpl; +import com.velocitypowered.api.event.connection.PreLoginEvent; +import com.velocitypowered.api.event.connection.PreLoginEvent.PreLoginComponentResult; +import com.velocitypowered.api.event.player.GameProfileRequestEvent; +import com.velocitypowered.api.proxy.InboundConnection; +import com.velocitypowered.api.util.GameProfile; +import com.velocitypowered.api.util.GameProfile.Property; +import java.lang.reflect.Field; +import java.util.Collections; +import java.util.List; +import java.util.UUID; +import java.util.stream.Collectors; +import net.kyori.adventure.text.Component; +import org.junit.jupiter.api.Test; + +/** + * The decision logic of Connect's defensive login re-assert. + * + *

Every case here is one of the fail-safe paths the design depends on: the re-assert only + * ever acts on Connect's own sessions, never overrides another plugin's kick, never fires when + * nothing changed the decision, and is fully switched off by + * {@code login-reassert.enabled: false}. + * + *

Ordering — that these handlers actually run after every other plugin's — is proven against + * the real Velocity dispatcher in {@code VelocityLateEventOrderTest} + * ({@code velocity/src/eventOrderTest}). + */ +class VelocityLateReassertListenerTest { + private static final UUID CONNECT_UUID = UUID.fromString("f912bf90-8349-565f-9dc0-9891923c0cc3"); + private static final UUID PROXY_UUID = UUID.fromString("11111111-2222-3333-4444-555555555555"); + + @Test + void preLoginReassertsOfflineModeWhenAnotherPluginForcedOnlineMode() throws Exception { + Fixture fixture = Fixture.connectSession(true, false); + PreLoginEvent event = fixture.preLogin(PreLoginComponentResult.forceOnlineMode()); + + fixture.listener.onPreLoginLate(event); + + assertTrue(event.getResult().isForceOfflineMode(), + "Connect must restore its own pre-login decision after other plugins ran"); + assertTrue(event.getResult().isAllowed()); + } + + @Test + void preLoginNeverOverridesAnotherPluginsKick() throws Exception { + Fixture fixture = Fixture.connectSession(true, false); + PreLoginComponentResult denied = + PreLoginComponentResult.denied(Component.text("banned")); + PreLoginEvent event = fixture.preLogin(denied); + + fixture.listener.onPreLoginLate(event); + + assertSame(denied, event.getResult(), "a deny must survive the re-assert untouched"); + assertFalse(event.getResult().isAllowed()); + } + + @Test + void preLoginIsANoOpWhenNobodyChangedConnectsDecision() throws Exception { + Fixture fixture = Fixture.connectSession(true, false); + PreLoginComponentResult offline = PreLoginComponentResult.forceOfflineMode(); + PreLoginEvent event = fixture.preLogin(offline); + + fixture.listener.onPreLoginLate(event); + + assertSame(offline, event.getResult(), "nothing to re-assert, so nothing may be written"); + } + + @Test + void preLoginIgnoresConnectionsConnectDidNotAuthenticate() throws Exception { + Fixture fixture = Fixture.foreignSession(true, false); + PreLoginComponentResult online = PreLoginComponentResult.forceOnlineMode(); + PreLoginEvent event = fixture.preLogin(online); + + fixture.listener.onPreLoginLate(event); + + assertSame(online, event.getResult(), + "a connection Connect did not authenticate is none of Connect's business"); + } + + @Test + void preLoginDoesNothingWhenTheOffSwitchIsDisabled() throws Exception { + Fixture fixture = Fixture.connectSession(false, false); + PreLoginComponentResult online = PreLoginComponentResult.forceOnlineMode(); + PreLoginEvent event = fixture.preLogin(online); + + fixture.listener.onPreLoginLate(event); + + assertSame(online, event.getResult(), + "login-reassert.enabled: false must restore the pre-fix behaviour exactly"); + } + + @Test + void gameProfileRestoresOnlyThePropertiesByDefault() throws Exception { + Fixture fixture = Fixture.connectSession(true, false); + GameProfileRequestEvent event = fixture.gameProfileRequest(); + + fixture.listener.onGameProfileRequestLate(event); + + assertEquals(PROXY_UUID, event.getGameProfile().getId(), + "the default must not take the UUID away from a login plugin that keys on it"); + assertEquals("RewrittenByAnotherPlugin", event.getGameProfile().getName()); + assertEquals(Collections.singletonList("textures"), propertyNames(event.getGameProfile())); + } + + @Test + void gameProfileRestoresTheFullProfileWhenOptedIn() throws Exception { + Fixture fixture = Fixture.connectSession(true, true); + GameProfileRequestEvent event = fixture.gameProfileRequest(); + + fixture.listener.onGameProfileRequestLate(event); + + assertEquals(CONNECT_UUID, event.getGameProfile().getId()); + assertEquals("ConnectSteve", event.getGameProfile().getName()); + assertEquals(Collections.singletonList("textures"), propertyNames(event.getGameProfile())); + } + + @Test + void gameProfileDoesNothingWhenTheOffSwitchIsDisabled() throws Exception { + Fixture fixture = Fixture.connectSession(false, true); + GameProfileRequestEvent event = fixture.gameProfileRequest(); + GameProfile before = event.getGameProfile(); + + fixture.listener.onGameProfileRequestLate(event); + + assertSame(before, event.getGameProfile()); + } + + /** + * The normal case with no conflicting plugin installed: the profile is already the one + * Connect's own EARLY handler produced, so the re-assert must write nothing at all - and in + * particular must not append a second copy of Connect's properties. + */ + @Test + void gameProfileIsANoOpWhenNobodyChangedIt() throws Exception { + Fixture fixture = Fixture.connectSession(true, true); + GameProfileRequestEvent event = fixture.gameProfileRequest(); + // What VelocityListener#onGameProfileRequest already did at PostOrder.EARLY. + event.setGameProfile( + VelocityGameProfiles.fromConnectPlayer(event.getGameProfile(), connectPlayer())); + GameProfile afterConnectsEarlyHandler = event.getGameProfile(); + + fixture.listener.onGameProfileRequestLate(event); + + assertSame(afterConnectsEarlyHandler, event.getGameProfile(), + "nothing changed the profile, so the re-assert must not rewrite it"); + assertEquals(Collections.singletonList("textures"), + propertyNames(event.getGameProfile())); + } + + @Test + void gameProfileIgnoresConnectionsConnectDidNotAuthenticate() throws Exception { + Fixture fixture = Fixture.foreignSession(true, true); + GameProfileRequestEvent event = fixture.gameProfileRequest(); + GameProfile before = event.getGameProfile(); + + fixture.listener.onGameProfileRequestLate(event); + + assertSame(before, event.getGameProfile()); + } + + private static List propertyNames(GameProfile profile) { + return profile.getProperties().stream() + .map(Property::getName) + .collect(Collectors.toList()); + } + + private static final class Fixture { + final VelocityLateReassertListener listener; + final InboundConnection connection = mock(InboundConnection.class); + + private Fixture(boolean enabled, boolean restoreFullProfile, boolean connectSession) + throws Exception { + VelocityConnectPlayers players = new VelocityConnectPlayers(); + if (connectSession) { + players.remember(connection, connectPlayer()); + } + listener = new VelocityLateReassertListener( + players, config(enabled, restoreFullProfile), mock(ConnectLogger.class)); + } + + static Fixture connectSession(boolean enabled, boolean restoreFullProfile) throws Exception { + return new Fixture(enabled, restoreFullProfile, true); + } + + static Fixture foreignSession(boolean enabled, boolean restoreFullProfile) throws Exception { + return new Fixture(enabled, restoreFullProfile, false); + } + + PreLoginEvent preLogin(PreLoginComponentResult result) { + PreLoginEvent event = new PreLoginEvent(connection, "ConnectSteve"); + event.setResult(result); + return event; + } + + /** The profile as a login plugin left it: its own UUID and name, and no skin. */ + GameProfileRequestEvent gameProfileRequest() { + GameProfile rewritten = + new GameProfile(PROXY_UUID, "RewrittenByAnotherPlugin", Collections.emptyList()); + return new GameProfileRequestEvent(connection, rewritten, false); + } + } + + private static ConnectPlayer connectPlayer() { + return new ConnectPlayerImpl( + "session-1", + new com.minekube.connect.api.player.GameProfile( + "ConnectSteve", + CONNECT_UUID, + Collections.singletonList( + new com.minekube.connect.api.player.GameProfile.Property( + "textures", "skin-value", "skin-signature"))), + new Auth(false), + ""); + } + + /** + * The config classes are populated reflectively by the config loader and expose no setters, + * so build the wanted state the same way. + */ + private static ProxyConnectConfig config(boolean enabled, boolean restoreFullProfile) + throws Exception { + ProxyConnectConfig config = new ProxyConnectConfig(); + LoginReassertConfig reassert = config.getLoginReassert(); + set(reassert, "enabled", enabled); + set(reassert, "restoreFullProfile", restoreFullProfile); + return config; + } + + private static void set(Object target, String fieldName, boolean value) throws Exception { + Field field = target.getClass().getDeclaredField(fieldName); + field.setAccessible(true); + field.setBoolean(target, value); + } +} From dd54a045b687ddd664823f6b73b2ecd236506221 Mon Sep 17 00:00:00 2001 From: "no-mistakes[bot]" Date: Tue, 28 Jul 2026 11:11:05 +0200 Subject: [PATCH 2/5] no-mistakes(review): Contained late listener registration failures locally --- AGENTS.md | 4 +- docs/login-plugin-integration.md | 3 ++ .../VelocityListenerRegistration.java | 11 +++- .../module/VelocityPlatformModule.java | 5 +- .../VelocityListenerRegistrationTest.java | 50 +++++++++++++++++++ 5 files changed, 69 insertions(+), 4 deletions(-) create mode 100644 velocity/src/test/java/com/minekube/connect/listener/VelocityListenerRegistrationTest.java diff --git a/AGENTS.md b/AGENTS.md index 80d181c60..9e60702af 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -88,7 +88,9 @@ curl -I -L --fail https://github.com/minekube/connect-java/releases/download/ **Prerequisite for `restore-full-profile: true`:** every login plugin on the proxy must key > its own database on the Mojang UUID. For **LibreLogin** that means setting > **`new-uuid-creator: MOJANG`** in LibreLogin's config *first*. Its default (`CRACKED`) diff --git a/velocity/src/main/java/com/minekube/connect/listener/VelocityListenerRegistration.java b/velocity/src/main/java/com/minekube/connect/listener/VelocityListenerRegistration.java index a3037291b..c5e4d40aa 100644 --- a/velocity/src/main/java/com/minekube/connect/listener/VelocityListenerRegistration.java +++ b/velocity/src/main/java/com/minekube/connect/listener/VelocityListenerRegistration.java @@ -26,6 +26,7 @@ package com.minekube.connect.listener; import com.minekube.connect.VelocityPlugin; +import com.minekube.connect.api.logger.ConnectLogger; import com.minekube.connect.platform.listener.ListenerRegistration; import com.velocitypowered.api.event.EventManager; import lombok.RequiredArgsConstructor; @@ -34,13 +35,21 @@ public final class VelocityListenerRegistration implements ListenerRegistration { private final EventManager eventManager; private final VelocityPlugin plugin; + private final ConnectLogger logger; @Override public void register(Object listener) { if (listener instanceof VelocityLateReassertListener) { // Cannot be an annotated listener: @Subscribe has no way to express "after // PostOrder.LAST" on the Velocity API this plugin compiles against. - ((VelocityLateReassertListener) listener).register(eventManager, plugin); + try { + ((VelocityLateReassertListener) listener).register(eventManager, plugin); + } catch (Throwable throwable) { + logger.error( + "Could not register Connect's login re-assert handlers; " + + "Connect will behave as it did before the late floor existed", + throwable); + } return; } eventManager.register(plugin, listener); diff --git a/velocity/src/main/java/com/minekube/connect/module/VelocityPlatformModule.java b/velocity/src/main/java/com/minekube/connect/module/VelocityPlatformModule.java index cfa336a5f..4397814a8 100644 --- a/velocity/src/main/java/com/minekube/connect/module/VelocityPlatformModule.java +++ b/velocity/src/main/java/com/minekube/connect/module/VelocityPlatformModule.java @@ -98,8 +98,9 @@ public ConnectLogger logger(Logger logger, LanguageManager languageManager) { @Singleton public ListenerRegistration listenerRegistration( EventManager eventManager, - VelocityPlugin plugin) { - return new VelocityListenerRegistration(eventManager, plugin); + VelocityPlugin plugin, + ConnectLogger logger) { + return new VelocityListenerRegistration(eventManager, plugin, logger); } @Provides diff --git a/velocity/src/test/java/com/minekube/connect/listener/VelocityListenerRegistrationTest.java b/velocity/src/test/java/com/minekube/connect/listener/VelocityListenerRegistrationTest.java new file mode 100644 index 000000000..d5be5fe41 --- /dev/null +++ b/velocity/src/test/java/com/minekube/connect/listener/VelocityListenerRegistrationTest.java @@ -0,0 +1,50 @@ +package com.minekube.connect.listener; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.contains; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.ArgumentMatchers.same; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; + +import com.minekube.connect.api.logger.ConnectLogger; +import com.velocitypowered.api.event.EventHandler; +import com.velocitypowered.api.event.EventManager; +import com.velocitypowered.api.event.PostOrder; +import com.velocitypowered.api.event.connection.PreLoginEvent; +import org.junit.jupiter.api.Test; + +class VelocityListenerRegistrationTest { + @Test + void lateRegistrationContainsThrowableAndLogsDegradedBehaviour() { + EventManager eventManager = mock(EventManager.class); + ConnectLogger logger = mock(ConnectLogger.class); + AssertionError failure = new AssertionError("registration failed"); + doThrow(failure).when(eventManager).register( + any(), eq(PreLoginEvent.class), eq(PostOrder.LAST), + org.mockito.ArgumentMatchers.>any()); + + VelocityListenerRegistration registration = + new VelocityListenerRegistration(eventManager, null, logger); + + assertDoesNotThrow(() -> registration.register(new VelocityLateReassertListener())); + + verify(logger).error(contains("login re-assert"), same(failure)); + } + + @Test + void ordinaryListenerRegistrationIsUnchanged() { + EventManager eventManager = mock(EventManager.class); + ConnectLogger logger = mock(ConnectLogger.class); + Object listener = new Object(); + VelocityListenerRegistration registration = + new VelocityListenerRegistration(eventManager, null, logger); + + registration.register(listener); + + verify(eventManager).register(isNull(), same(listener)); + } +} From a738ff5c0a745d674f30217a356fb63703ca83ff Mon Sep 17 00:00:00 2001 From: "no-mistakes[bot]" Date: Tue, 28 Jul 2026 11:21:41 +0200 Subject: [PATCH 3/5] no-mistakes(review): Made late handler registration atomic with rollback --- .../VelocityLateReassertListener.java | 55 +++++++++++++++-- .../VelocityLateReassertListenerTest.java | 60 +++++++++++++++++++ 2 files changed, 110 insertions(+), 5 deletions(-) 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 516458a84..af1dc6fb3 100644 --- a/velocity/src/main/java/com/minekube/connect/listener/VelocityLateReassertListener.java +++ b/velocity/src/main/java/com/minekube/connect/listener/VelocityLateReassertListener.java @@ -29,6 +29,7 @@ import com.minekube.connect.api.logger.ConnectLogger; import com.minekube.connect.api.player.ConnectPlayer; import com.minekube.connect.config.ProxyConnectConfig; +import com.velocitypowered.api.event.EventHandler; import com.velocitypowered.api.event.EventManager; import com.velocitypowered.api.event.connection.PreLoginEvent; import com.velocitypowered.api.event.connection.PreLoginEvent.PreLoginComponentResult; @@ -92,11 +93,55 @@ public VelocityLateReassertListener() { */ public void register(EventManager eventManager, Object plugin) { VelocityLateEventRegistrar registrar = new VelocityLateEventRegistrar(eventManager, plugin); - boolean afterLast = registrar.registerAfterLast(PreLoginEvent.class, this::onPreLoginLate); - registrar.registerAfterLast(GameProfileRequestEvent.class, this::onGameProfileRequestLate); - logger.debug( - "Registered the login re-assert handlers (numeric priority: {})", - afterLast ? "yes" : "no, this Velocity only supports PostOrder.LAST"); + EventHandler preLoginHandler = this::onPreLoginLate; + EventHandler gameProfileHandler = this::onGameProfileRequestLate; + boolean preLoginRegistered = false; + boolean gameProfileRegistered = false; + try { + boolean afterLast = registrar.registerAfterLast(PreLoginEvent.class, preLoginHandler); + preLoginRegistered = true; + registrar.registerAfterLast(GameProfileRequestEvent.class, gameProfileHandler); + gameProfileRegistered = true; + logger.debug( + "Registered the login re-assert handlers (numeric priority: {})", + afterLast ? "yes" : "no, this Velocity only supports PostOrder.LAST"); + } catch (Throwable registrationFailure) { + if (gameProfileRegistered) { + unregisterSafely(eventManager, plugin, gameProfileHandler); + } + if (preLoginRegistered) { + unregisterSafely(eventManager, plugin, preLoginHandler); + } + rethrow(registrationFailure); + } + } + + private void unregisterSafely( + EventManager eventManager, Object plugin, EventHandler handler) { + try { + unregister(eventManager, plugin, handler); + } catch (Throwable rollbackFailure) { + try { + logger.error("Could not roll back a partially registered login re-assert handler", + rollbackFailure); + } catch (Throwable ignored) { + } + } + } + + private static void unregister( + EventManager eventManager, Object plugin, EventHandler handler) { + eventManager.unregister(plugin, handler); + } + + private static void rethrow(Throwable throwable) { + if (throwable instanceof RuntimeException) { + throw (RuntimeException) throwable; + } + if (throwable instanceof Error) { + throw (Error) throwable; + } + throw new RuntimeException(throwable); } void onPreLoginLate(PreLoginEvent event) { 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 85730daf3..7a71ac5c5 100644 --- a/velocity/src/test/java/com/minekube/connect/listener/VelocityLateReassertListenerTest.java +++ b/velocity/src/test/java/com/minekube/connect/listener/VelocityLateReassertListenerTest.java @@ -1,5 +1,6 @@ package com.minekube.connect.listener; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertSame; @@ -12,6 +13,9 @@ import com.minekube.connect.config.ProxyConnectConfig; import com.minekube.connect.config.ProxyConnectConfig.LoginReassertConfig; import com.minekube.connect.player.ConnectPlayerImpl; +import com.velocitypowered.api.event.EventHandler; +import com.velocitypowered.api.event.EventManager; +import com.velocitypowered.api.event.PostOrder; import com.velocitypowered.api.event.connection.PreLoginEvent; import com.velocitypowered.api.event.connection.PreLoginEvent.PreLoginComponentResult; import com.velocitypowered.api.event.player.GameProfileRequestEvent; @@ -19,9 +23,11 @@ import com.velocitypowered.api.util.GameProfile; import com.velocitypowered.api.util.GameProfile.Property; import java.lang.reflect.Field; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.UUID; +import java.util.concurrent.CompletableFuture; import java.util.stream.Collectors; import net.kyori.adventure.text.Component; import org.junit.jupiter.api.Test; @@ -171,6 +177,21 @@ void gameProfileIgnoresConnectionsConnectDidNotAuthenticate() throws Exception { assertSame(before, event.getGameProfile()); } + @Test + void partialRegistrationRollsBackOnlyTheHandlerThatWasRegistered() { + PartialFailureEventManager eventManager = new PartialFailureEventManager(); + ConnectLogger logger = mock(ConnectLogger.class); + VelocityListenerRegistration registration = + new VelocityListenerRegistration(eventManager, null, logger); + + assertDoesNotThrow(() -> registration.register(new VelocityLateReassertListener())); + + assertEquals(1, eventManager.registeredHandlers.size()); + assertEquals(1, eventManager.unregisteredHandlers.size()); + assertSame(eventManager.registeredHandlers.get(0), eventManager.unregisteredHandlers.get(0)); + assertFalse(eventManager.unregisterListenersCalled); + } + private static List propertyNames(GameProfile profile) { return profile.getProperties().stream() .map(Property::getName) @@ -244,4 +265,43 @@ private static void set(Object target, String fieldName, boolean value) throws E field.setAccessible(true); field.setBoolean(target, value); } + + private static final class PartialFailureEventManager implements EventManager { + final List> registeredHandlers = new ArrayList<>(); + final List> unregisteredHandlers = new ArrayList<>(); + boolean unregisterListenersCalled; + + @Override + public void register(Object plugin, Object listener) { + throw new AssertionError("late handlers must use explicit registration"); + } + + @Override + public void register(Object plugin, Class eventClass, PostOrder postOrder, + EventHandler handler) { + if (!registeredHandlers.isEmpty()) { + throw new AssertionError("second late-handler registration failed"); + } + registeredHandlers.add(handler); + } + + @Override + public CompletableFuture fire(E event) { + return CompletableFuture.completedFuture(event); + } + + @Override + public void unregisterListeners(Object plugin) { + unregisterListenersCalled = true; + } + + @Override + public void unregisterListener(Object plugin, Object listener) { + } + + @Override + public void unregister(Object plugin, EventHandler handler) { + unregisteredHandlers.add(handler); + } + } } From 4f0506eaa07166714a54591eea150b3f009921a3 Mon Sep 17 00:00:00 2001 From: "no-mistakes[bot]" Date: Tue, 28 Jul 2026 11:32:57 +0200 Subject: [PATCH 4/5] no-mistakes(review): Cached Velocity feature detection per registrar --- .../listener/VelocityLateEventRegistrar.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/velocity/src/main/java/com/minekube/connect/listener/VelocityLateEventRegistrar.java b/velocity/src/main/java/com/minekube/connect/listener/VelocityLateEventRegistrar.java index 5b366c16a..d2bd524bf 100644 --- a/velocity/src/main/java/com/minekube/connect/listener/VelocityLateEventRegistrar.java +++ b/velocity/src/main/java/com/minekube/connect/listener/VelocityLateEventRegistrar.java @@ -42,9 +42,10 @@ * {@link #AFTER_LAST} takes that slot, so the handler runs after every {@code LAST} * handler no matter which plugin loaded first. The * {@code register(Object, Class, short, EventHandler)} overload that reaches it only - * exists on Velocity builds from 2024-09-16 onwards, which is why it is looked up - * reflectively - on a public interface method, so its absence is a plain - * {@link NoSuchMethodException} rather than anything that can break class loading. + * exists on Velocity builds from 2024-09-16 onwards, which is why one feature-detection + * lookup is performed per registrar instance - reflectively on a public interface method, + * so its absence is a plain {@link NoSuchMethodException} rather than anything that can + * break class loading. *
  • Load order. On older builds the handler falls back to {@code PostOrder.LAST}. * Velocity breaks ties between equal orders by registration order, which is plugin load * order, which is a topological sort of the declared dependency graph - so the @@ -66,10 +67,12 @@ final class VelocityLateEventRegistrar { private final EventManager eventManager; private final Object plugin; + private final Method shortRegisterMethod; VelocityLateEventRegistrar(EventManager eventManager, Object plugin) { this.eventManager = eventManager; this.plugin = plugin; + this.shortRegisterMethod = shortRegisterMethod(); } /** @@ -92,10 +95,9 @@ static Method shortRegisterMethod() { * was registered at {@link PostOrder#LAST} instead and relies on load order */ boolean registerAfterLast(Class eventType, EventHandler handler) { - Method shortRegister = shortRegisterMethod(); - if (shortRegister != null) { + if (shortRegisterMethod != null) { try { - shortRegister.invoke(eventManager, plugin, eventType, AFTER_LAST, handler); + shortRegisterMethod.invoke(eventManager, plugin, eventType, AFTER_LAST, handler); return true; } catch (ReflectiveOperationException | LinkageError ignored) { // Fall through to the load-order lever below. From d649b4d26d1fb5beb6c65c4fbc811cf305872014 Mon Sep 17 00:00:00 2001 From: "no-mistakes[bot]" Date: Tue, 28 Jul 2026 11:53:34 +0200 Subject: [PATCH 5/5] no-mistakes(document): Clarify legacy Velocity ordering fallback documentation --- AGENTS.md | 8 ++++-- docs/login-plugin-integration.md | 26 ++++++++++--------- .../listener/VelocityLateEventRegistrar.java | 7 +++-- .../VelocityLateReassertListener.java | 17 +++++++----- 4 files changed, 35 insertions(+), 23 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9e60702af..53d00dc64 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -83,8 +83,12 @@ curl -I -L --fail https://github.com/minekube/connect-java/releases/download/after every other plugin's handler. + * Registers event handlers that should run strictly after every other plugin's handler + * when the running Velocity supports numeric priorities, with a load-order fallback for older + * Velocity builds. * *

    Two layered levers, because neither covers the whole range of Velocity versions Connect * supports on its own: @@ -89,7 +91,8 @@ static Method shortRegisterMethod() { } /** - * Registers {@code handler} so that it runs after every other handler of {@code eventType}. + * Registers {@code handler} after every other handler of {@code eventType} when the numeric + * priority overload is available; otherwise it uses the load-order fallback. * * @return whether the numeric-priority lever was available; {@code false} means the handler * was registered at {@link PostOrder#LAST} instead and relies on load order 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 af1dc6fb3..8dfc57de0 100644 --- a/velocity/src/main/java/com/minekube/connect/listener/VelocityLateReassertListener.java +++ b/velocity/src/main/java/com/minekube/connect/listener/VelocityLateReassertListener.java @@ -39,7 +39,10 @@ import java.util.List; /** - * Re-asserts Connect's own login decision after every other plugin has had its say. + * Re-asserts Connect's own login decision after every other plugin has had its say where the + * running Velocity supports strict-after event ordering. Older Velocity builds fall back to + * {@code PostOrder.LAST} and the optional plugin load-order edge described by + * {@link VelocityLateEventRegistrar}. * *

    Connect authenticates a tunneled player at the Minekube edge and hands the proxy an * already-verified, offline-mode connection ({@link VelocityListener}'s @@ -59,15 +62,15 @@ * * *

    Because the trigger is Connect's own observed state, this covers every plugin that forces - * online mode after Connect - LibreLogin, AuthMe, nLogin, anything - without Connect reading, - * linking against, or version-checking any of them. + * online mode after Connect - LibreLogin, AuthMe, nLogin, anything - without Connect's runtime + * code reading, linking against, or version-checking any of them. * *

    By default only the profile properties (the skin) are restored, not the UUID: * login plugins commonly key their own database on the UUID the proxy ends up with, and * changing it out from under them breaks their lookups. Restoring the full profile is * available as an opt-in, with its prerequisite documented in {@code proxy-config.yml}. * - * @see VelocityLateEventRegistrar for how "after every other plugin" is achieved + * @see VelocityLateEventRegistrar for how late ordering is selected */ public final class VelocityLateReassertListener { @Inject private VelocityConnectPlayers connectPlayers; @@ -87,9 +90,9 @@ public VelocityLateReassertListener() { } /** - * Registers both handlers after every other plugin's. Called instead of the usual annotated - * listener registration, because {@code @Subscribe} cannot express "after {@code LAST}" on - * the Velocity API Connect compiles against. + * Registers both handlers as late as the running Velocity supports. Called instead of the + * usual annotated listener registration, because {@code @Subscribe} cannot express "after + * {@code LAST}" on the Velocity API Connect compiles against. */ public void register(EventManager eventManager, Object plugin) { VelocityLateEventRegistrar registrar = new VelocityLateEventRegistrar(eventManager, plugin);