Close the gap for duplicate login casing (#892)

* Close the gap for duplicate login casing

* Undo unrelated formatting change

* Refactor kick before registering into separate method

* Rewrite registerConnection & friends

* Extract PlayerRegistry with per-identity lock across login and disconnect phase

* Apply various fixes after PR review

---------

Co-authored-by: Wouter Gritter <wouter@gritter.nl>

Author: R00tB33rMan

proxy/src/main/java/com/velocitypowered/proxy/VelocityServer.java

@@ -75,6 +75,7 @@
 import com.velocitypowered.proxy.config.ProxyAddress;
 import com.velocitypowered.proxy.config.VelocityConfiguration;
 import com.velocitypowered.proxy.connection.client.ConnectedPlayer;
+import com.velocitypowered.proxy.connection.client.PlayerRegistry;
 import com.velocitypowered.proxy.connection.player.resourcepack.VelocityResourcePackInfo;
 import com.velocitypowered.proxy.connection.util.ServerListPingHandler;
 import com.velocitypowered.proxy.console.VelocityConsole;
@@ -112,14 +113,12 @@
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
-import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.UUID;
 import java.util.concurrent.CompletableFuture;
-import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
@@ -204,9 +203,7 @@ public class VelocityServer implements ProxyServer, ForwardingAudience {
 
   private final VelocityPluginManager pluginManager;
 
-  private final Map<UUID, ConnectedPlayer> connectionsByUuid = new ConcurrentHashMap<>();
-
-  private final Map<String, ConnectedPlayer> connectionsByName = new ConcurrentHashMap<>();
+  private final PlayerRegistry playerRegistry = new PlayerRegistry(this);
 
   /**
    * Holds a set of all registered BuiltinCommand instances. Used for unregistering these commands later.
@@ -882,8 +879,10 @@ public void shutdown(boolean explicitExit, Component reason) {
         LOGGER.warn("Interrupted while waiting for ProxyPreShutdownEvent; continuing shutdown.");
       }
 
-      ImmutableList<@NotNull ConnectedPlayer> players = ImmutableList.copyOf(connectionsByUuid.values());
-      ImmutableList<@NotNull UUID> playerUuids = ImmutableList.copyOf(connectionsByUuid.keySet());
+      ImmutableList<@NotNull ConnectedPlayer> players = ImmutableList.copyOf(playerRegistry.getPlayers());
+      ImmutableList<@NotNull UUID> playerUuids = players.stream()
+          .map(ConnectedPlayer::getUniqueId)
+          .collect(ImmutableList.toImmutableList());
 
       if (!getConfiguration().isAcceptTransfers()) {
         for (ConnectedPlayer player : players) {
@@ -954,6 +953,8 @@ public void shutdown(boolean explicitExit, Component reason) {
         this.redis.shutdown();
       }
 
+      this.playerRegistry.shutdown();
+
       // Since we manually removed the shutdown hook, we need to handle the shutdown ourselves.
       LogManager.shutdown();
 
@@ -1057,112 +1058,32 @@ public ConnectionManager getConnectionManager() {
     return tabCompleteRateLimiter;
   }
 
-  /**
-   * Checks if a connection identified by the given profile can be registered with the proxy.
-   * Called before the {@link ConnectedPlayer} instance is constructed so that rejected
-   * connections never create a phantom player object.
-   *
-   * @param profile the resolved game profile of the incoming connection
-   * @return {@code true} if we can register the connection, {@code false} if not
-   */
-  public boolean canRegisterConnection(GameProfile profile) {
-    // When kick-existing-players is enabled, skip duplicate checks here.
-    // registerConnection() handles kicking the existing player and enforcing IP rules.
-    if (configuration.isKickExistingPlayers()) {
-      return true;
-    }
-
-    String lowerName = profile.getName().toLowerCase(Locale.US);
-
-    if (connectionsByName.containsKey(lowerName)) {
-      return false;
-    }
-
-    return !connectionsByUuid.containsKey(profile.getId());
+  public PlayerRegistry getPlayerRegistry() {
+    return playerRegistry;
   }
 
   /**
-   * Attempts to register the {@code connection} with the proxy.
+   * Attempts to register the {@code connection} with the proxy, kicking any existing
+   * player under the same name or UUID if present and if
+   * {@link VelocityConfiguration#isKickExistingPlayers()} is {@code true}.
    *
    * @param connection the connection to register
-   * @return {@code true} if we registered the connection, {@code false} if not
+   * @return a future resolving to {@code true} if we registered the connection, {@code false} if not
    */
-  public boolean registerConnection(ConnectedPlayer connection) {
-    String lowerName = connection.getUsername().toLowerCase(Locale.US);
-
-    if (!this.configuration.isKickExistingPlayers()) {
-      // Standard behavior: block duplicate connections
-      if (connectionsByName.containsKey(lowerName)) {
-        return false;
-      }
-
-      connectionsByName.put(lowerName, connection);
-
-      if (connectionsByUuid.putIfAbsent(connection.getUniqueId(), connection) != null) {
-        connectionsByName.remove(lowerName, connection);
-        return false;
-      }
-
-      return true;
-    }
-
-    // kick-existing-players is enabled. Kick the existing session so the new one can take over.
-    // When kick-existing-players-check-ip is also enabled, only kick if the new connection comes
-    // from the same IP address.
-    ConnectedPlayer existingByUuid = connectionsByUuid.get(connection.getUniqueId());
-    if (existingByUuid != null) {
-      if (this.configuration.isKickExistingPlayersCheckIp()) {
-        InetAddress newIp = connection.getRemoteAddress().getAddress();
-        InetAddress existingIp = existingByUuid.getRemoteAddress().getAddress();
-        if (!newIp.equals(existingIp)) {
-          // Different IP with same UUID: protect the existing player, deny the new connection.
-          return false;
-        }
-      }
-      existingByUuid.disconnect(Component.translatable("multiplayer.disconnect.duplicate_login"));
-    }
-
-    // Also check for a username conflict whose UUID differs from the one above.
-    ConnectedPlayer existingByName = connectionsByName.get(lowerName);
-    if (existingByName != null && existingByName != existingByUuid) {
-      if (this.configuration.isKickExistingPlayersCheckIp()) {
-        InetAddress newIp = connection.getRemoteAddress().getAddress();
-        InetAddress existingIp = existingByName.getRemoteAddress().getAddress();
-        if (!newIp.equals(existingIp)) {
-          return false;
-        }
-      }
-      existingByName.disconnect(Component.translatable("multiplayer.disconnect.duplicate_login"));
-    }
-
-    connectionsByName.put(lowerName, connection);
-    connectionsByUuid.put(connection.getUniqueId(), connection);
-
-    return true;
-  }
-
-  /**
-   * Unregisters the given player from the proxy.
-   *
-   * @param connection the connection to unregister
-   */
-  public void unregisterConnection(ConnectedPlayer connection) {
-    connectionsByName.remove(connection.getUsername().toLowerCase(Locale.US), connection);
-    connectionsByUuid.remove(connection.getUniqueId(), connection);
-    connection.disconnected();
+  public CompletableFuture<Boolean> registerConnection(ConnectedPlayer connection) {
+    return playerRegistry.registerConnection(connection);
   }
 
   @Override
   public Optional<ConnectedPlayer> getPlayer(String username) {
     Preconditions.checkNotNull(username, "username");
-
-    return Optional.ofNullable(connectionsByName.get(username.toLowerCase(Locale.US)));
+    return playerRegistry.getPlayer(username);
   }
 
   @Override
   public Optional<ConnectedPlayer> getPlayer(UUID uuid) {
     Preconditions.checkNotNull(uuid, "uuid");
-    return Optional.ofNullable(connectionsByUuid.get(uuid));
+    return playerRegistry.getPlayer(uuid);
   }
 
   @Override
@@ -1185,12 +1106,12 @@ public Collection<VelocityRegisteredServer> matchServer(String partialName) {
 
   @Override
   public Collection<ConnectedPlayer> getAllPlayers() {
-    return ImmutableList.copyOf(connectionsByUuid.values());
+    return ImmutableList.copyOf(playerRegistry.getPlayers());
   }
 
   @Override
   public @UnmodifiableView Collection<ConnectedPlayer> getOnlinePlayers() {
-    return Collections.unmodifiableCollection(connectionsByUuid.values());
+    return playerRegistry.getPlayers();
   }
 
   /**
@@ -1202,7 +1123,7 @@ public Collection<ConnectedPlayer> getAllPlayers() {
    */
   @SuppressWarnings("BooleanMethodIsAlwaysInverted")
   public boolean isPlayerOnline(ConnectedPlayer player) {
-    return connectionsByUuid.get(player.getUniqueId()) == player;
+    return playerRegistry.isPlayerOnline(player);
   }
 
   @Override
@@ -1218,7 +1139,7 @@ public int getPlayerCount() {
    * @return the number of locally connected players
    */
   public int getLocalPlayerCount() {
-    return connectionsByUuid.size();
+    return playerRegistry.getLocalPlayerCount();
   }
 
   @Override

proxy/src/main/java/com/velocitypowered/proxy/connection/client/AuthSessionHandler.java

@@ -20,7 +20,6 @@
 import static com.velocitypowered.api.network.ProtocolVersion.MINECRAFT_1_8;
 
 import com.google.common.base.Preconditions;
-import com.velocitypowered.api.event.connection.DisconnectEvent;
 import com.velocitypowered.api.event.connection.LoginEvent;
 import com.velocitypowered.api.event.connection.PostLoginEvent;
 import com.velocitypowered.api.event.permission.PermissionsSetupEvent;
@@ -142,46 +141,46 @@ public void activated() {
 
       GameProfile resolvedProfile = profileEvent.getGameProfile();
 
-      if (!server.canRegisterConnection(resolvedProfile)) {
-        inbound.disconnect(
-            Component.translatable("velocity.error.already-connected-proxy", NamedTextColor.RED));
-        return CompletableFuture.completedFuture(null);
-      }
-
-      // Initiate a regular connection and move over to it.
+      // Initiate a regular connection and move over to it. Duplicate-detection and IP-check
+      // rejection happen atomically inside server.registerConnection() under the identity
+      // lock; if rejected, the !registered branch below disconnects the player. Because
+      // LoginEvent never fires for a rejected player, the natural teardown path does not
+      // fire a spurious DisconnectEvent (see PlayerRegistry#fireDisconnectAndCleanup).
       ConnectedPlayer player = new ConnectedPlayer(server, resolvedProfile,
           mcConnection, inbound.getVirtualHost().orElse(null), inbound.getRawVirtualHost().orElse(null), onlineMode,
           inbound.getHandshakeIntent(), inbound.getIdentifiedKey());
       this.connectedPlayer = player;
 
-      if (!server.registerConnection(player)) {
-        player.disconnect0(
-            Component.translatable("velocity.error.already-connected-proxy", NamedTextColor.RED),
-            true);
-        return CompletableFuture.completedFuture(null);
-      }
+      return server.registerConnection(player).thenComposeAsync(registered -> {
+        if (!registered) {
+          player.disconnect0(
+              Component.translatable("velocity.error.already-connected-proxy", NamedTextColor.RED),
+              true);
+          return CompletableFuture.completedFuture(null);
+        }
 
-      if (server.getConfiguration().isLogPlayerConnections()) {
-        LOGGER.info("{} has connected", player);
-      }
+        if (server.getConfiguration().isLogPlayerConnections()) {
+          LOGGER.info("{} has connected", player);
+        }
 
-      return server.getEventManager()
-          .fire(new PermissionsSetupEvent(player, ConnectedPlayer.DEFAULT_PERMISSIONS))
-          .thenAcceptAsync(event -> {
-            if (!mcConnection.isClosed()) {
-              // wait for permissions to load, then set the players permission function
-              PermissionFunction function = event.createFunction(player);
-              if (function == null) {
-                LOGGER.error("A plugin permission provider {} provided an invalid permission "
-                        + "function for player {}. This is a bug in the plugin, not in "
-                        + "Velocity. Falling back to the default permission function.",
-                    event.getProvider().getClass().getName(), player.getUsername());
-              } else {
-                player.setPermissionFunction(function);
+        return server.getEventManager()
+            .fire(new PermissionsSetupEvent(player, ConnectedPlayer.DEFAULT_PERMISSIONS))
+            .thenAcceptAsync(event -> {
+              if (!mcConnection.isClosed()) {
+                // wait for permissions to load, then set the players permission function
+                PermissionFunction function = event.createFunction(player);
+                if (function == null) {
+                  LOGGER.error("A plugin permission provider {} provided an invalid permission "
+                          + "function for player {}. This is a bug in the plugin, not in "
+                          + "Velocity. Falling back to the default permission function.",
+                      event.getProvider().getClass().getName(), player.getUsername());
+                } else {
+                  player.setPermissionFunction(function);
+                }
+                startLoginCompletion(player);
               }
-              startLoginCompletion(player);
-            }
-          }, mcConnection.eventLoop());
+            }, mcConnection.eventLoop());
+      }, mcConnection.eventLoop());
     }, mcConnection.eventLoop()).exceptionally((ex) -> {
       LOGGER.error("Exception during connection of {}", finalProfile, ex);
       return null;
@@ -256,9 +255,12 @@ public boolean handle(LoginAcknowledgedPacket packet) {
       loginState = State.ACKNOWLEDGED;
       mcConnection.setActiveSessionHandler(StateRegistry.CONFIG, new ClientConfigSessionHandler(server, connectedPlayer));
 
-      server.getEventManager().fire(new PostLoginEvent(connectedPlayer)).thenCompose(ignored -> connectToInitialServer(connectedPlayer))
+      ConnectedPlayer player = connectedPlayer;
+      server.getEventManager().fire(new PostLoginEvent(player))
+          .whenComplete((ignored, throwable) -> server.getPlayerRegistry().finalizeLogin(player))
+          .thenCompose(ignored -> connectToInitialServer(player))
           .exceptionally((ex) -> {
-            LOGGER.error("Exception while connecting {} to initial server", connectedPlayer, ex);
+            LOGGER.error("Exception while connecting {} to initial server", player, ex);
             return null;
           });
     }
@@ -288,11 +290,11 @@ public boolean handle(ServerboundCookieResponsePacket packet) {
   private void completeLoginProtocolPhaseAndInitialize(ConnectedPlayer player) {
     mcConnection.setAssociation(player);
 
+    player.markLoginEventFired();
     server.getEventManager().fire(new LoginEvent(player, serverIdHash)).thenAcceptAsync(event -> {
       if (mcConnection.isClosed()) {
-        // The player was disconnected
-        server.getEventManager().fireAndForget(new DisconnectEvent(player,
-            DisconnectEvent.LoginStatus.CANCELLED_BY_USER_BEFORE_COMPLETE));
+        // The player was disconnected during LoginEvent processing. The natural teardown path
+        // is in flight (or will be) and will fire DisconnectEvent + release the identity lock.
         return;
       }
 
@@ -316,8 +318,10 @@ private void completeLoginProtocolPhaseAndInitialize(ConnectedPlayer player) {
         if (inbound.getProtocolVersion().lessThan(ProtocolVersion.MINECRAFT_1_20_2)) {
           loginState = State.ACKNOWLEDGED;
           mcConnection.setActiveSessionHandler(StateRegistry.PLAY, new InitialConnectSessionHandler(player, server));
-          server.getEventManager().fire(new PostLoginEvent(player)).thenCompose((ignored) ->
-              connectToInitialServer(player)).exceptionally((ex) -> {
+          server.getEventManager().fire(new PostLoginEvent(player))
+              .whenComplete((ignored, throwable) -> server.getPlayerRegistry().finalizeLogin(player))
+              .thenCompose((ignored) -> connectToInitialServer(player))
+              .exceptionally((ex) -> {
                 LOGGER.error("Exception while connecting {} to initial server", player, ex);
                 return null;
               });