fix(paper): deny a pre-join connection whose name claim was taken over

Author: Xephi

authme-paper-common/src/main/java/fr/xephi/authme/listener/PaperLoginValidationListener.java

@@ -28,8 +28,8 @@ public class PaperLoginValidationListener implements Listener {
 
     private static final LegacyComponentSerializer LEGACY_SERIALIZER = LegacyComponentSerializer.legacySection();
 
-    /** Extra time a name stays claimed on top of the configured login / register timeout. */
-    private static final long CLAIM_GRACE_MILLIS = TimeUnit.SECONDS.toMillis(10);
+    /** 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);
 
     @Inject
     private OnJoinVerifier onJoinVerifier;
@@ -110,8 +110,18 @@ private void verifyConfigurationPhase(PlayerConnectionValidateLoginEvent event,
                                           PlayerConfigurationConnection connection) {
         PlayerProfile profile = connection.getProfile();
         String playerName = profile == null ? null : profile.getName();
-        // No claim means an online player being reconfigured, which must not be checked against itself
-        if (playerName == null || !pendingConnectionRegistry.holdsClaim(playerName, connection)) {
+        if (playerName == null) {
+            return;
+        }
+
+        // This connection held the name to get here, so losing it means another one took it over
+        if (pendingConnectionRegistry.isClaimedByOtherConnection(playerName, connection)) {
+            denyConnection(event, playerName, MessageKey.USERNAME_ALREADY_ONLINE_ERROR);
+            return;
+        }
+
+        // No claim at all means an online player being reconfigured, never checked against itself
+        if (!pendingConnectionRegistry.holdsClaim(playerName, connection)) {
             return;
         }
 

authme-paper-common/src/main/java/fr/xephi/authme/service/PendingConnectionRegistry.java

@@ -33,6 +33,11 @@ public boolean holdsClaim(String name, PlayerConnection connection) {
         return claim != null && !claim.isStale() && claim.isHeldBy(connectionKey(connection));
     }
 
+    public boolean isClaimedByOtherConnection(String name, PlayerConnection connection) {
+        Claim claim = claims.get(normalize(name));
+        return claim != null && !claim.isStale() && !claim.isHeldBy(connectionKey(connection));
+    }
+
     public void release(String name) {
         claims.remove(normalize(name));
     }

authme-paper-common/src/test/java/fr/xephi/authme/listener/PaperLoginValidationListenerTest.java

@@ -115,6 +115,20 @@ public void shouldIgnoreReconfigurationOfPlayerWithoutClaim() {
         assertThat(event.isAllowed(), is(true));
     }
 
+    @Test
+    public void shouldKickWhenClaimWasTakenOverDuringConfiguration() {
+        PlayerConfigurationConnection connection = newConfigurationConnection("Bobby");
+        given(pendingConnectionRegistry.isClaimedByOtherConnection("Bobby", connection)).willReturn(true);
+        givenAlreadyOnlineMessage();
+        PlayerConnectionValidateLoginEvent event = new PlayerConnectionValidateLoginEvent(connection, null);
+
+        listener.onPlayerConnectionValidateLogin(event);
+
+        assertThat(event.isAllowed(), is(false));
+        assertThat(serialize(event.getKickMessage()), is("&cAlready online"));
+        verifyNoInteractions(onJoinVerifier);
+    }
+
     @Test
     public void shouldVerifySingleSessionWhenFinishingConfiguration() throws FailedVerificationException {
         givenConfiguredTimeouts();

authme-paper-common/src/test/java/fr/xephi/authme/service/PendingConnectionRegistryTest.java

@@ -53,6 +53,31 @@ public void shouldRefuseSecondLiveConnectionWithSameName() {
         assertThat(registry.holdsClaim("Bobby", first), is(true));
     }
 
+    @Test
+    public void shouldReportNameClaimedByAnotherLiveConnection() {
+        PlayerConnection holder = newConnection(50_000);
+        PlayerConnection other = newConnection(50_001);
+        registry.tryClaim("Bobby", holder, TTL);
+
+        assertThat(registry.isClaimedByOtherConnection("Bobby", other), is(true));
+        assertThat(registry.isClaimedByOtherConnection("Bobby", holder), is(false));
+    }
+
+    @Test
+    public void shouldNotReportStaleOrMissingClaimAsHeldByAnotherConnection() {
+        PlayerConnection other = newConnection(50_001);
+
+        assertThat(registry.isClaimedByOtherConnection("Bobby", other), is(false));
+
+        PlayerConnection gone = newConnection(50_000);
+        registry.tryClaim("Bobby", gone, TTL);
+        given(gone.isConnected()).willReturn(false);
+        assertThat(registry.isClaimedByOtherConnection("Bobby", other), is(false));
+
+        registry.tryClaim("Alice", newConnection(50_002), -1L);
+        assertThat(registry.isClaimedByOtherConnection("Alice", other), is(false));
+    }
+
     @Test
     public void shouldAllowSameConnectionToRenewItsClaim() {
         PlayerConnection connection = newConnection(50_000);