Skip to content

Commit 90296bb

Browse files
robinbraemerno-mistakes[bot]
robinbraemer
and
no-mistakes[bot]
authored
fix(proxy): re-assert Connect's login decision after other plugins (#77)
* 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. * no-mistakes(review): Contained late listener registration failures locally * no-mistakes(review): Made late handler registration atomic with rollback * no-mistakes(review): Cached Velocity feature detection per registrar * no-mistakes(document): Clarify legacy Velocity ordering fallback documentation --------- Co-authored-by: no-mistakes[bot] <robin.braemer+no-mistakes-bot@web.de>
1 parent fa7c3d9 commit 90296bb

28 files changed

Lines changed: 1852 additions & 45 deletions

‎AGENTS.md‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,39 @@ curl -I -L --fail https://github.com/minekube/connect-java/releases/download/<ve
8080
commitment). Pinned by
8181
`core/.../network/netty/ConnectPlayerAttributeBoundaryTest`.
8282

83+
## Defensive login re-assert (proxy)
84+
85+
- Connect registers a **second, late** pre-login handler that restores its own decision after
86+
every other plugin has run where the platform supports strict-after ordering, fixing the whole
87+
"a login plugin forces online mode after Connect" class (LibreLogin, AuthMe, nLogin) with no
88+
runtime dependency on any of them. On older Velocity builds, the fallback is `PostOrder.LAST`
89+
plus the optional LibreLogin load-order edge, so other plugins retain the old last-writer
90+
behavior.
91+
Preserve the property
92+
that makes it safe: it reacts **only** to Connect's own result on the event object, never
93+
reads/links against/version-checks a third-party plugin, never overrides a deny, and no-ops
94+
when nothing changed the decision. The existing `EARLY`/`LOWEST` handlers stay as they are -
95+
this only adds a floor. If late-handler registration throws, `VelocityListenerRegistration`
96+
catches `Throwable` locally, logs the failure, and continues with the pre-existing behavior;
97+
ordinary listener registration remains unchanged. Authoritative:
98+
`velocity/.../listener/VelocityLateEventRegistrar.java` (the two layered ordering levers and
99+
why each exists), `VelocityLateReassertListener`, `BungeeLateReassertListener`,
100+
`core/src/main/resources/proxy-config.yml` (`login-reassert`), `docs/login-plugin-integration.md`.
101+
- **Do not bump velocity-api past `3.2.0-SNAPSHOT`** to reach `PostOrder.CUSTOM`: Velocity reads
102+
the annotation while collecting a listener's methods, so an enum constant an older runtime
103+
lacks throws `EnumConstantNotPresentException` there and kills *all* of Connect's handlers on
104+
pre-2024-09-16 proxies, unguardably. The reflective short-`register` lookup buys the same
105+
ordering with a catchable failure.
106+
- Default profile scope is properties-only (skin) deliberately; restoring Connect's UUID breaks
107+
login plugins that key their storage on the proxy UUID, so it is opt-in with its
108+
`new-uuid-creator: MOJANG` prerequisite documented next to the option.
109+
- `:velocity:eventOrderTest` is a separate source set running the **real** `VelocityEventManager`
110+
and `PluginDependencyUtils` against a Velocity proxy jar pinned by sha256 (ivy repo in
111+
`settings.gradle.kts`; PaperMC publishes no proxy artifact to Maven). Separate because that
112+
shaded jar carries its own velocity-api and `com.velocitypowered.proxy` classes, which must not
113+
shadow the 3.2.0 API or the stubs in `velocity/src/test`. It runs under `check`, so a
114+
fill-data.papermc.io outage fails `./gradlew build` until the artifact resolves or is cached.
115+
83116
## Injector Scoping (config availability)
84117

85118
- The parent injector binds `ConfigHolder`; `ConnectPlatform.init()` populates it

‎bungee/src/main/java/com/minekube/connect/listener/BungeeConnections.java‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
/*
2+
* Copyright (c) 2021-2022 Minekube. https://minekube.com
3+
*
4+
* Permission is hereby granted, free of charge, to any person obtaining a copy
5+
* of this software and associated documentation files (the "Software"), to deal
6+
* in the Software without restriction, including without limitation the rights
7+
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
8+
* copies of the Software, and to permit persons to whom the Software is
9+
* furnished to do so, subject to the following conditions:
10+
*
11+
* The above copyright notice and this permission notice shall be included in
12+
* all copies or substantial portions of the Software.
13+
*
14+
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
15+
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
16+
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
17+
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
18+
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
19+
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
20+
* THE SOFTWARE.
21+
*
22+
* @author Minekube
23+
* @link https://github.com/minekube/connect-java
24+
*/
25+
26+
package com.minekube.connect.listener;
27+
28+
import static com.google.common.base.Preconditions.checkNotNull;
29+
30+
import com.minekube.connect.util.ReflectionUtils;
31+
import io.netty.channel.Channel;
32+
import java.lang.reflect.Field;
33+
import net.md_5.bungee.api.connection.PendingConnection;
34+
import net.md_5.bungee.connection.InitialHandler;
35+
import net.md_5.bungee.netty.ChannelWrapper;
36+
37+
/**
38+
* The BungeeCord internals Connect needs during login, which its public API does not expose.
39+
*/
40+
final class BungeeConnections {
41+
private static final Field CHANNEL_WRAPPER;
42+
private static final Field PLAYER_NAME;
43+
44+
static {
45+
CHANNEL_WRAPPER =
46+
ReflectionUtils.getFieldOfType(InitialHandler.class, ChannelWrapper.class);
47+
checkNotNull(CHANNEL_WRAPPER, "ChannelWrapper field cannot be null");
48+
49+
PLAYER_NAME = ReflectionUtils.getField(InitialHandler.class, "name");
50+
checkNotNull(PLAYER_NAME, "Initial name field cannot be null");
51+
}
52+
53+
private BungeeConnections() {
54+
}
55+
56+
static Channel channel(PendingConnection connection) {
57+
ChannelWrapper wrapper = ReflectionUtils.getCastedValue(connection, CHANNEL_WRAPPER);
58+
return wrapper.getHandle();
59+
}
60+
61+
static void setName(PendingConnection connection, String name) {
62+
ReflectionUtils.setValue(connection, PLAYER_NAME, name);
63+
}
64+
}

‎bungee/src/main/java/com/minekube/connect/listener/BungeeLateReassertListener.java‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/*
2+
* Copyright (c) 2021-2022 Minekube. https://minekube.com
3+
*
4+
* Permission is hereby granted, free of charge, to any person obtaining a copy
5+
* of this software and associated documentation files (the "Software"), to deal
6+
* in the Software without restriction, including without limitation the rights
7+
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
8+
* copies of the Software, and to permit persons to whom the Software is
9+
* furnished to do so, subject to the following conditions:
10+
*
11+
* The above copyright notice and this permission notice shall be included in
12+
* all copies or substantial portions of the Software.
13+
*
14+
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
15+
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
16+
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
17+
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
18+
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
19+
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
20+
* THE SOFTWARE.
21+
*
22+
* @author Minekube
23+
* @link https://github.com/minekube/connect-java
24+
*/
25+
26+
package com.minekube.connect.listener;
27+
28+
import com.google.inject.Inject;
29+
import com.minekube.connect.api.logger.ConnectLogger;
30+
import com.minekube.connect.api.player.ConnectPlayer;
31+
import com.minekube.connect.config.ProxyConnectConfig;
32+
import com.minekube.connect.network.netty.LocalSession;
33+
import net.md_5.bungee.api.connection.PendingConnection;
34+
import net.md_5.bungee.api.event.PreLoginEvent;
35+
import net.md_5.bungee.api.plugin.Listener;
36+
import net.md_5.bungee.event.EventHandler;
37+
38+
/**
39+
* The BungeeCord counterpart of {@code VelocityLateReassertListener}: re-asserts Connect's own
40+
* pre-login decision after every other plugin has had its say, so that a login plugin forcing
41+
* online mode on a Connect-tunneled connection cannot leave the player hanging at
42+
* "Logging in..." forever.
43+
*
44+
* <p>{@link EventHandler#priority()} is a plain {@code byte} and BungeeCord's event bus
45+
* dispatches the whole byte range, so {@link Byte#MAX_VALUE} runs strictly after
46+
* {@code EventPriority.HIGHEST} (64). That is the only correct lever here: BungeeCord breaks
47+
* ties between equal priorities with a {@code HashMap} keyed by listener identity, so
48+
* registration order - and therefore any {@code softDepends} - decides nothing.
49+
*
50+
* <p>Same guarantees as on Velocity: only connections Connect itself authenticated are touched,
51+
* another plugin's kick is never overridden, and {@code login-reassert.enabled: false} turns
52+
* the whole thing off.
53+
*
54+
* <p>BungeeCord's {@code PendingConnection} exposes no profile-properties API at pre-login, so
55+
* the profile half of this is limited to the UUID and username, behind the same opt-in
56+
* {@code login-reassert.restore-full-profile}.
57+
*/
58+
public final class BungeeLateReassertListener implements Listener {
59+
@Inject private ProxyConnectConfig config;
60+
@Inject private ConnectLogger logger;
61+
62+
@EventHandler(priority = Byte.MAX_VALUE)
63+
public void onPreLoginLate(PreLoginEvent event) {
64+
if (!config.getLoginReassert().isEnabled()) {
65+
return;
66+
}
67+
if (event.isCancelled()) {
68+
return; // a plugin denied this login; never convert a kick into a join
69+
}
70+
try {
71+
PendingConnection connection = event.getConnection();
72+
LocalSession.context(BungeeConnections.channel(connection),
73+
ctx -> reassert(connection, ctx.getPlayer()));
74+
} catch (Exception exception) {
75+
// Never let the defensive floor itself break a login: without it the connection is
76+
// exactly where it would have been before this listener existed.
77+
logger.error("Failed to re-assert Connect's pre-login decision", exception);
78+
}
79+
}
80+
81+
private void reassert(PendingConnection connection, ConnectPlayer player) {
82+
if (player.getAuth().isPassthrough()) {
83+
return; // not authenticated by Connect - none of our business
84+
}
85+
if (connection.isOnlineMode()) {
86+
connection.setOnlineMode(false);
87+
logger.debug("Re-asserted offline mode for Connect session {} at pre-login; another "
88+
+ "plugin had changed it (set login-reassert.enabled to false to allow that)",
89+
player.getUsername());
90+
}
91+
if (!config.getLoginReassert().isRestoreFullProfile()) {
92+
return;
93+
}
94+
if (!player.getUniqueId().equals(connection.getUniqueId())) {
95+
connection.setUniqueId(player.getUniqueId());
96+
}
97+
if (!player.getUsername().equals(connection.getName())) {
98+
BungeeConnections.setName(connection, player.getUsername());
99+
}
100+
}
101+
}

‎bungee/src/main/java/com/minekube/connect/listener/BungeeListener.java‎

Lines changed: 2 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,6 @@
2525

2626
package com.minekube.connect.listener;
2727

28-
import static com.google.common.base.Preconditions.checkNotNull;
29-
3028
import com.google.inject.Inject;
3129
import com.minekube.connect.api.ProxyConnectApi;
3230
import com.minekube.connect.api.logger.ConnectLogger;
@@ -35,34 +33,17 @@
3533
import com.minekube.connect.bedrock.BedrockIdentityEnforcer.Decision;
3634
import com.minekube.connect.network.netty.LocalSession;
3735
import com.minekube.connect.util.LanguageManager;
38-
import com.minekube.connect.util.ReflectionUtils;
39-
import io.netty.channel.Channel;
40-
import java.lang.reflect.Field;
4136
import java.util.UUID;
4237
import net.md_5.bungee.api.connection.PendingConnection;
4338
import net.md_5.bungee.api.event.LoginEvent;
4439
import net.md_5.bungee.api.event.PlayerDisconnectEvent;
4540
import net.md_5.bungee.api.event.PreLoginEvent;
4641
import net.md_5.bungee.api.plugin.Listener;
47-
import net.md_5.bungee.connection.InitialHandler;
4842
import net.md_5.bungee.event.EventHandler;
4943
import net.md_5.bungee.event.EventPriority;
50-
import net.md_5.bungee.netty.ChannelWrapper;
5144

5245
@SuppressWarnings("ConstantConditions")
5346
public final class BungeeListener implements Listener {
54-
private static final Field CHANNEL_WRAPPER;
55-
private static final Field PLAYER_NAME;
56-
57-
static {
58-
CHANNEL_WRAPPER =
59-
ReflectionUtils.getFieldOfType(InitialHandler.class, ChannelWrapper.class);
60-
checkNotNull(CHANNEL_WRAPPER, "ChannelWrapper field cannot be null");
61-
62-
PLAYER_NAME = ReflectionUtils.getField(InitialHandler.class, "name");
63-
checkNotNull(PLAYER_NAME, "Initial name field cannot be null");
64-
}
65-
6647
@Inject private ProxyConnectApi api;
6748
@Inject private LanguageManager languageManager;
6849
@Inject private ConnectLogger logger;
@@ -77,10 +58,7 @@ public void onPreLogin(PreLoginEvent event) {
7758

7859
PendingConnection connection = event.getConnection();
7960

80-
ChannelWrapper wrapper = ReflectionUtils.getCastedValue(connection, CHANNEL_WRAPPER);
81-
Channel channel = wrapper.getHandle();
82-
83-
LocalSession.context(channel, ctx -> {
61+
LocalSession.context(BungeeConnections.channel(connection), ctx -> {
8462
Decision decision = bedrockIdentityEnforcer.verify(ctx);
8563
if (!decision.allowed()) {
8664
bedrockIdentityEnforcer.reject(ctx, decision);
@@ -90,7 +68,7 @@ public void onPreLogin(PreLoginEvent event) {
9068
}
9169
connection.setOnlineMode(false);
9270
connection.setUniqueId(ctx.getPlayer().getUniqueId());
93-
ReflectionUtils.setValue(connection, PLAYER_NAME, ctx.getPlayer().getUsername());
71+
BungeeConnections.setName(connection, ctx.getPlayer().getUsername());
9472
// TODO robin: what about profile properties? (but why is skin already showing)
9573
});
9674
}

‎bungee/src/main/java/com/minekube/connect/module/BungeeListenerModule.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import com.google.inject.Singleton;
3030
import com.google.inject.TypeLiteral;
3131
import com.google.inject.multibindings.ProvidesIntoSet;
32+
import com.minekube.connect.listener.BungeeLateReassertListener;
3233
import com.minekube.connect.listener.BungeeListener;
3334
import com.minekube.connect.register.ListenerRegister;
3435
import net.md_5.bungee.api.plugin.Listener;
@@ -45,4 +46,10 @@ public Listener bungeeListener() {
4546
return new BungeeListener();
4647
}
4748

49+
@Singleton
50+
@ProvidesIntoSet
51+
public Listener bungeeLateReassertListener() {
52+
return new BungeeLateReassertListener();
53+
}
54+
4855
}

‎bungee/src/test/java/com/minekube/connect/BungeePluginStartupTest.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import com.minekube.connect.bedrock.BedrockIdentityKeyProvider;
4242
import com.minekube.connect.inject.CommonPlatformInjector;
4343
import com.minekube.connect.inject.bungee.BungeeInjector;
44+
import com.minekube.connect.listener.BungeeLateReassertListener;
4445
import com.minekube.connect.listener.BungeeListener;
4546
import com.minekube.connect.listener.BungeeListenerRegistration;
4647
import com.minekube.connect.module.ProxyCommonModule;
@@ -80,6 +81,7 @@ class BungeePluginStartupTest {
8081
private static List<Class<?>> bungeeGraphRoots() {
8182
List<Class<?>> roots = new ArrayList<>(StartupGraphProvisioning.coreRuntimeGraphRoots());
8283
roots.add(BungeeListener.class);
84+
roots.add(BungeeLateReassertListener.class);
8385
roots.add(BungeeCommandUtil.class);
8486
roots.add(BungeePlatformUtils.class);
8587
roots.add(BungeeInjector.class);

‎bungee/src/test/java/com/minekube/connect/listener/BungeeLateReassertListenerTest.java‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
package com.minekube.connect.listener;
2+
3+
import static org.junit.jupiter.api.Assertions.assertEquals;
4+
import static org.junit.jupiter.api.Assertions.assertTrue;
5+
6+
import java.lang.reflect.Method;
7+
import net.md_5.bungee.api.event.PreLoginEvent;
8+
import net.md_5.bungee.event.EventHandler;
9+
import net.md_5.bungee.event.EventPriority;
10+
import org.junit.jupiter.api.Test;
11+
12+
/**
13+
* Pins BungeeCord's half of the ordering lever.
14+
*
15+
* <p>{@code EventPriority} is not an enum - it is a set of {@code byte} constants, the highest of
16+
* which is {@code HIGHEST = 64}, while {@code EventHandler#priority()} is a plain {@code byte}
17+
* and BungeeCord's event bus bakes handlers across the entire byte range. So
18+
* {@link Byte#MAX_VALUE} is inside the annotation's declared domain and runs strictly after
19+
* {@code HIGHEST}.
20+
*
21+
* <p>This is the only correct lever on BungeeCord: handlers of equal priority live in a
22+
* {@code HashMap} keyed by listener identity, so the tie-break is not registration order and no
23+
* {@code softDepends} in {@code plugin.yml} can influence it - unlike on Velocity, where the
24+
* declared dependency graph is a real fallback.
25+
*/
26+
class BungeeLateReassertListenerTest {
27+
@Test
28+
void theReassertRunsAfterEveryOtherPluginsPreLoginHandler() throws Exception {
29+
Method handler =
30+
BungeeLateReassertListener.class.getMethod("onPreLoginLate", PreLoginEvent.class);
31+
32+
byte priority = handler.getAnnotation(EventHandler.class).priority();
33+
34+
assertEquals(Byte.MAX_VALUE, priority,
35+
"the re-assert must run after HIGHEST; nothing lower is a floor");
36+
assertTrue(priority > EventPriority.HIGHEST);
37+
}
38+
39+
/**
40+
* The re-assert only adds a floor. Connect's original pre-login handler keeps running first,
41+
* so Connect's ordering relative to every other plugin is otherwise unchanged.
42+
*/
43+
@Test
44+
void theOriginalPreLoginHandlerStillRunsFirst() throws Exception {
45+
Method original = BungeeListener.class.getMethod("onPreLogin", PreLoginEvent.class);
46+
47+
assertEquals(EventPriority.LOWEST, original.getAnnotation(EventHandler.class).priority());
48+
}
49+
}

‎core/src/main/java/com/minekube/connect/config/ProxyConnectConfig.java‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,4 +32,33 @@
3232
*/
3333
@Getter
3434
public final class ProxyConnectConfig extends ConnectConfig {
35+
private static final LoginReassertConfig DEFAULT_LOGIN_REASSERT = new LoginReassertConfig();
36+
37+
/**
38+
* Whether Connect re-asserts its own pre-login decision after every other plugin has run.
39+
*/
40+
private LoginReassertConfig loginReassert = new LoginReassertConfig();
41+
42+
/**
43+
* Never {@code null}: a config file written before this option existed keeps the defaults.
44+
*/
45+
public LoginReassertConfig getLoginReassert() {
46+
return loginReassert != null ? loginReassert : DEFAULT_LOGIN_REASSERT;
47+
}
48+
49+
@Getter
50+
public static class LoginReassertConfig {
51+
/**
52+
* Default on. Turning it off restores the behaviour of leaving Connect's pre-login
53+
* decision to whichever plugin writes last, which is what an operator who deliberately
54+
* wants another plugin to override Connect needs.
55+
*/
56+
private boolean enabled = true;
57+
/**
58+
* Default off. Also restores Connect's UUID and username, not just the skin properties.
59+
* Requires every login plugin on the proxy to key its own storage on the Mojang UUID -
60+
* see the prerequisite documented next to this option in {@code proxy-config.yml}.
61+
*/
62+
private boolean restoreFullProfile = false;
63+
}
3564
}

0 commit comments

Comments
 (0)