From 6d1fb42bba80358f1a1851a08f7cc923ce947a13 Mon Sep 17 00:00:00 2001 From: Rishabh Venu Date: Sat, 10 Oct 2026 05:51:11 -0400 Subject: [PATCH] fix(dialog): keep the pre-join dialog open when the timeout is 0 loginTimeout and registerTimeout are documented as "Set to 0 to disable" and the post-join timeout task honours that. The pre-join dialog clamped the value to at least one second instead, so with 0 the dialog closed after one second and the player was kicked with the timeout message. Only schedule the timeout when the setting is positive. Two things that relied on the timeout always firing are adjusted for the unlimited wait: - the configuration thread waiting for the answer is released when the connection closes, otherwise it would stay parked for good - the pending name claim no longer expires 60 seconds into the dialog, which let a second connection take over the name and got the first one refused as already online once it answered Fixes #3122 --- .../listener/PaperDialogFlowListener.java | 27 +++-- .../PaperLoginValidationListener.java | 12 +- .../listener/PaperDialogFlowListenerTest.java | 108 +++++++++++++++++- .../PaperLoginValidationListenerTest.java | 14 +++ 4 files changed, 149 insertions(+), 12 deletions(-) diff --git a/authme-paper-common/src/main/java/fr/xephi/authme/listener/PaperDialogFlowListener.java b/authme-paper-common/src/main/java/fr/xephi/authme/listener/PaperDialogFlowListener.java index e39fda0ea4..9344ffc0df 100644 --- a/authme-paper-common/src/main/java/fr/xephi/authme/listener/PaperDialogFlowListener.java +++ b/authme-paper-common/src/main/java/fr/xephi/authme/listener/PaperDialogFlowListener.java @@ -206,17 +206,26 @@ public void onPlayerConnectionClose(PlayerConnectionCloseEvent event) { private void retireSession(PlayerConfigurationConnection connection) { Long sessionId = connectionSessions.remove(connection); if (sessionId != null) { - pendingLoginResponses.remove(sessionId); - pendingRegisterResponses.remove(sessionId); + releaseResponse(pendingLoginResponses.remove(sessionId)); + releaseResponse(pendingRegisterResponses.remove(sessionId)); preJoinDialogService.retireSession(sessionId); } } + // Without a timeout nothing else wakes the configuration thread waiting on a connection that is gone + private static void releaseResponse(CompletableFuture response) { + if (response != null) { + response.complete(null); + } + } + private void handleBlockingLoginDialog(PlayerConfigurationConnection connection, long sessionId, String playerName) { CompletableFuture loginResponse = new CompletableFuture<>(); - long timeoutSeconds = Math.max(commonService.getProperty(RestrictionSettings.LOGIN_TIMEOUT), 1); - loginResponse.completeOnTimeout( - messages.retrieveSingle(playerName, MessageKey.LOGIN_TIMEOUT_ERROR), timeoutSeconds, TimeUnit.SECONDS); + int timeoutSeconds = commonService.getProperty(RestrictionSettings.LOGIN_TIMEOUT); + if (timeoutSeconds > 0) { + loginResponse.completeOnTimeout( + messages.retrieveSingle(playerName, MessageKey.LOGIN_TIMEOUT_ERROR), timeoutSeconds, TimeUnit.SECONDS); + } String normalizedName = playerName.toLowerCase(Locale.ROOT); pendingLoginResponses.put(sessionId, loginResponse); preJoinDialogService.registerPreJoinFuture(sessionId, loginResponse); @@ -284,9 +293,11 @@ private void processPreJoinRecoverySubmit(long sessionId, DialogResponseView dia private void handleBlockingRegisterDialog(PlayerConfigurationConnection connection, long sessionId, String playerName, Dialog dialog) { CompletableFuture registerResponse = new CompletableFuture<>(); - long timeoutSeconds = Math.max(commonService.getProperty(RestrictionSettings.REGISTER_TIMEOUT), 1); - registerResponse.completeOnTimeout( - messages.retrieveSingle(playerName, MessageKey.LOGIN_TIMEOUT_ERROR), timeoutSeconds, TimeUnit.SECONDS); + int timeoutSeconds = commonService.getProperty(RestrictionSettings.REGISTER_TIMEOUT); + if (timeoutSeconds > 0) { + registerResponse.completeOnTimeout( + messages.retrieveSingle(playerName, MessageKey.LOGIN_TIMEOUT_ERROR), timeoutSeconds, TimeUnit.SECONDS); + } pendingRegisterResponses.put(sessionId, registerResponse); connection.getAudience().showDialog(dialog); diff --git a/authme-paper-common/src/main/java/fr/xephi/authme/listener/PaperLoginValidationListener.java b/authme-paper-common/src/main/java/fr/xephi/authme/listener/PaperLoginValidationListener.java index df71ac5f62..6a70b722cb 100644 --- a/authme-paper-common/src/main/java/fr/xephi/authme/listener/PaperLoginValidationListener.java +++ b/authme-paper-common/src/main/java/fr/xephi/authme/listener/PaperLoginValidationListener.java @@ -31,6 +31,9 @@ public class PaperLoginValidationListener implements Listener { /** Safety net for a connection that never reports itself as gone; not the release mechanism. */ private static final long CLAIM_GRACE_MILLIS = TimeUnit.SECONDS.toMillis(60); + /** Used when a timeout is disabled: the pre-join dialog then stays open for as long as the connection does. */ + private static final long CLAIM_WITHOUT_TIMEOUT_MILLIS = TimeUnit.DAYS.toMillis(1); + @Inject private OnJoinVerifier onJoinVerifier; @@ -142,9 +145,12 @@ private void denyConnection(PlayerConnectionValidateLoginEvent event, String pla } private long getClaimTtlMillis() { - int timeoutSeconds = Math.max(settings.getProperty(RestrictionSettings.LOGIN_TIMEOUT), - settings.getProperty(RestrictionSettings.REGISTER_TIMEOUT)); - return TimeUnit.SECONDS.toMillis(Math.max(timeoutSeconds, 0)) + CLAIM_GRACE_MILLIS; + int loginTimeout = settings.getProperty(RestrictionSettings.LOGIN_TIMEOUT); + int registerTimeout = settings.getProperty(RestrictionSettings.REGISTER_TIMEOUT); + if (loginTimeout <= 0 || registerTimeout <= 0) { + return CLAIM_WITHOUT_TIMEOUT_MILLIS; + } + return TimeUnit.SECONDS.toMillis(Math.max(loginTimeout, registerTimeout)) + CLAIM_GRACE_MILLIS; } private static String getPlayerName(PlayerLoginConnection connection) { diff --git a/authme-paper-common/src/test/java/fr/xephi/authme/listener/PaperDialogFlowListenerTest.java b/authme-paper-common/src/test/java/fr/xephi/authme/listener/PaperDialogFlowListenerTest.java index 313ee35c25..c6c756d001 100644 --- a/authme-paper-common/src/test/java/fr/xephi/authme/listener/PaperDialogFlowListenerTest.java +++ b/authme-paper-common/src/test/java/fr/xephi/authme/listener/PaperDialogFlowListenerTest.java @@ -1,5 +1,6 @@ package fr.xephi.authme.listener; +import ch.jalu.configme.properties.Property; import com.destroystokyo.paper.event.player.PlayerConnectionCloseEvent; import com.destroystokyo.paper.profile.PlayerProfile; import fr.xephi.authme.data.ProxySessionManager; @@ -24,6 +25,7 @@ import fr.xephi.authme.settings.properties.RegistrationSettings; import fr.xephi.authme.settings.properties.RestrictionSettings; import io.papermc.paper.connection.PlayerConfigurationConnection; +import io.papermc.paper.dialog.Dialog; import io.papermc.paper.dialog.DialogResponseView; import io.papermc.paper.event.connection.configuration.AsyncPlayerConnectionConfigureEvent; import io.papermc.paper.event.player.PlayerCustomClickEvent; @@ -42,6 +44,7 @@ import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.TimeUnit; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; @@ -49,6 +52,7 @@ import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.BDDMockito.given; +import static org.mockito.BDDMockito.willAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.never; @@ -503,10 +507,11 @@ public void shouldRetireSessionOfClosedConnectionOnly() throws Exception { given(gone.isConnected()).willReturn(false); PlayerConfigurationConnection live = mockConnection("Bobby"); given(live.isConnected()).willReturn(true); + CompletableFuture goneFuture = new CompletableFuture<>(); CompletableFuture liveFuture = new CompletableFuture<>(); connectionSessions(listener).put(gone, SESSION_ID); connectionSessions(listener).put(live, SESSION_ID + 1); - pendingResponses(listener, "pendingLoginResponses").put(SESSION_ID, new CompletableFuture<>()); + pendingResponses(listener, "pendingLoginResponses").put(SESSION_ID, goneFuture); pendingResponses(listener, "pendingLoginResponses").put(SESSION_ID + 1, liveFuture); listener.onPlayerConnectionClose(new PlayerConnectionCloseEvent( @@ -517,6 +522,107 @@ public void shouldRetireSessionOfClosedConnectionOnly() throws Exception { assertThat(connectionSessions(listener).containsKey(gone), is(false)); assertThat(connectionSessions(listener).get(live), is(SESSION_ID + 1)); assertThat(pendingResponses(listener, "pendingLoginResponses").get(SESSION_ID + 1), is(liveFuture)); + assertThat("thread waiting on the closed connection must be released", goneFuture.isDone(), is(true)); + assertThat(liveFuture.isDone(), is(false)); + } + + @Test + public void shouldKeepPreJoinLoginDialogOpenWhenLoginTimeoutIsDisabled() throws Exception { + PaperDialogFlowListener listener = new PaperDialogFlowListener(); + PreJoinDialogService preJoinDialogService = mock(PreJoinDialogService.class); + Audience audience = mock(Audience.class); + PlayerConfigurationConnection connection = newBlockingDialogSetup(listener, preJoinDialogService, audience); + setField(listener, "proxySessionManager", mock(ProxySessionManager.class)); + setField(listener, "dialogWindowService", mock(DialogWindowService.class)); + givenTimeout(listener, RestrictionSettings.LOGIN_TIMEOUT, 0); + givenDialogIsAnsweredLate(listener, audience, "pendingLoginResponses"); + + try (var helperStatic = mockStatic(PaperDialogHelper.class)) { + helperStatic.when(() -> PaperDialogHelper.createPreJoinLoginDialog(any())).thenReturn(null); + + invokeHandleBlockingLoginDialog(listener, connection); + } + + verify(preJoinDialogService, never()).storePendingKickMessage(anyLong(), anyString()); + } + + @Test + public void shouldKeepPreJoinRegisterDialogOpenWhenRegisterTimeoutIsDisabled() throws Exception { + PaperDialogFlowListener listener = new PaperDialogFlowListener(); + PreJoinDialogService preJoinDialogService = mock(PreJoinDialogService.class); + Audience audience = mock(Audience.class); + PlayerConfigurationConnection connection = newBlockingDialogSetup(listener, preJoinDialogService, audience); + givenTimeout(listener, RestrictionSettings.REGISTER_TIMEOUT, 0); + givenDialogIsAnsweredLate(listener, audience, "pendingRegisterResponses"); + + invokeHandleBlockingRegisterDialog(listener, connection); + + verify(preJoinDialogService, never()).storePendingKickMessage(anyLong(), anyString()); + } + + @Test + public void shouldKickWhenPreJoinLoginDialogTimesOut() throws Exception { + PaperDialogFlowListener listener = new PaperDialogFlowListener(); + PreJoinDialogService preJoinDialogService = mock(PreJoinDialogService.class); + PlayerConfigurationConnection connection = + newBlockingDialogSetup(listener, preJoinDialogService, mock(Audience.class)); + setField(listener, "proxySessionManager", mock(ProxySessionManager.class)); + setField(listener, "dialogWindowService", mock(DialogWindowService.class)); + givenTimeout(listener, RestrictionSettings.LOGIN_TIMEOUT, 1); + + try (var helperStatic = mockStatic(PaperDialogHelper.class)) { + helperStatic.when(() -> PaperDialogHelper.createPreJoinLoginDialog(any())).thenReturn(null); + + invokeHandleBlockingLoginDialog(listener, connection); + } + + verify(preJoinDialogService).storePendingKickMessage(SESSION_ID, "Timed out!"); + } + + private static PlayerConfigurationConnection newBlockingDialogSetup(PaperDialogFlowListener listener, + PreJoinDialogService preJoinDialogService, + Audience audience) throws Exception { + Messages messages = mock(Messages.class); + setField(listener, "messages", messages); + setField(listener, "preJoinDialogService", preJoinDialogService); + given(messages.retrieveSingle("Bobby", MessageKey.LOGIN_TIMEOUT_ERROR)).willReturn("Timed out!"); + + PlayerConfigurationConnection connection = mockConnection("Bobby"); + given(connection.getAudience()).willReturn(audience); + return connection; + } + + private static void givenTimeout(PaperDialogFlowListener listener, Property property, + int seconds) throws Exception { + CommonService commonService = mock(CommonService.class); + setField(listener, "commonService", commonService); + given(commonService.getProperty(property)).willReturn(seconds); + } + + // The player answers after 1.2 seconds; a disabled timeout used to end the wait after one second + private static void givenDialogIsAnsweredLate(PaperDialogFlowListener listener, Audience audience, + String responseField) { + willAnswer(invocation -> { + pendingResponses(listener, responseField).get(SESSION_ID) + .completeOnTimeout(null, 1200, TimeUnit.MILLISECONDS); + return null; + }).given(audience).showDialog(any()); + } + + private static void invokeHandleBlockingLoginDialog(PaperDialogFlowListener listener, + PlayerConfigurationConnection connection) throws Exception { + Method method = PaperDialogFlowListener.class.getDeclaredMethod("handleBlockingLoginDialog", + PlayerConfigurationConnection.class, long.class, String.class); + method.setAccessible(true); + method.invoke(listener, connection, SESSION_ID, "Bobby"); + } + + private static void invokeHandleBlockingRegisterDialog(PaperDialogFlowListener listener, + PlayerConfigurationConnection connection) throws Exception { + Method method = PaperDialogFlowListener.class.getDeclaredMethod("handleBlockingRegisterDialog", + PlayerConfigurationConnection.class, long.class, String.class, Dialog.class); + method.setAccessible(true); + method.invoke(listener, connection, SESSION_ID, "Bobby", null); } private static PlayerConfigurationConnection mockConnection(String playerName) { diff --git a/authme-paper-common/src/test/java/fr/xephi/authme/listener/PaperLoginValidationListenerTest.java b/authme-paper-common/src/test/java/fr/xephi/authme/listener/PaperLoginValidationListenerTest.java index 7cbb047d0e..05e90f290f 100644 --- a/authme-paper-common/src/test/java/fr/xephi/authme/listener/PaperLoginValidationListenerTest.java +++ b/authme-paper-common/src/test/java/fr/xephi/authme/listener/PaperLoginValidationListenerTest.java @@ -25,6 +25,7 @@ import java.net.InetAddress; import java.util.UUID; +import java.util.concurrent.TimeUnit; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; @@ -74,6 +75,19 @@ public void shouldClaimNameDuringLoginPhase() { verify(pendingConnectionRegistry).tryClaim(eq("Bobby"), eq(connection), anyLong()); } + @Test + public void shouldClaimNameForLongerThanTheGracePeriodWhenTimeoutIsDisabled() { + given(settings.getProperty(RestrictionSettings.LOGIN_TIMEOUT)).willReturn(0); + given(settings.getProperty(RestrictionSettings.REGISTER_TIMEOUT)).willReturn(30); + PlayerLoginConnection connection = newLoginConnection("Bobby"); + given(pendingConnectionRegistry.tryClaim(eq("Bobby"), eq(connection), anyLong())).willReturn(true); + PlayerConnectionValidateLoginEvent event = new PlayerConnectionValidateLoginEvent(connection, null); + + listener.onPlayerConnectionValidateLogin(event); + + verify(pendingConnectionRegistry).tryClaim("Bobby", connection, TimeUnit.DAYS.toMillis(1)); + } + @Test public void shouldKickDuplicateSessionOnValidationEvent() throws FailedVerificationException { PlayerLoginConnection connection = newLoginConnection("Bobby");