diff --git a/AGENTS.md b/AGENTS.md index 2d18afb95..53d00dc64 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -80,6 +80,39 @@ 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..ab256d9e6 100644 --- a/docs/login-plugin-integration.md +++ b/docs/login-plugin-integration.md @@ -110,6 +110,84 @@ 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 restores its own decision if something changed it. +Where the platform supports a strict-after ordering, that handler runs **after every other +plugin's** handler. 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's runtime code does not read, link against, or + version-check any login plugin, so this covers every plugin with this behaviour, present and + future. The legacy Velocity load-order edge is descriptor metadata, not a runtime integration. + +### 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. + +If late-handler registration fails, Connect logs the failure, skips the re-assert floor, and +continues with its pre-existing handlers. Ordinary listener registration is unaffected. + +> **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. On older builds it falls back to `PostOrder.LAST` plus the `optional` `librelogin` + dependency in `velocity-plugin.json`: equal orders then follow plugin load order, so this + preserves the intended tie-break for LibreLogin, while a different or absent plugin keeps + the old last-writer behavior. 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 +213,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 +230,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..ebaeb290a --- /dev/null +++ b/velocity/src/main/java/com/minekube/connect/listener/VelocityLateEventRegistrar.java @@ -0,0 +1,112 @@ +/* + * 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 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: + * + *

    + *
  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 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.
    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; + private final Method shortRegisterMethod; + + VelocityLateEventRegistrar(EventManager eventManager, Object plugin) { + this.eventManager = eventManager; + this.plugin = plugin; + this.shortRegisterMethod = shortRegisterMethod(); + } + + /** + * 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} 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 + */ + boolean registerAfterLast(Class eventType, EventHandler handler) { + if (shortRegisterMethod != null) { + try { + shortRegisterMethod.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..8dfc57de0 --- /dev/null +++ b/velocity/src/main/java/com/minekube/connect/listener/VelocityLateReassertListener.java @@ -0,0 +1,217 @@ +/* + * 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.EventHandler; +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 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 + * {@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'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 late ordering is selected + */ +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 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); + 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) { + 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..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,9 +35,23 @@ 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. + 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/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/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/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..7a71ac5c5 --- /dev/null +++ b/velocity/src/test/java/com/minekube/connect/listener/VelocityLateReassertListenerTest.java @@ -0,0 +1,307 @@ +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; +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.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; +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.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; + +/** + * 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()); + } + + @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) + .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); + } + + 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); + } + } +} 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)); + } +}