fix: only allow one connection per account

Previously, if someone connected to the server twice with the same
account, the server would abort because two entities cannot have the
same name in flecs. In addition, having multiple players from the same
account would not make sense. This code disconnects the previous player
if a new player with the same name connects.

This also modifies IgnMap to no longer use DeferredMap. Deferred
insertions would allow for race conditions because two players with the
same name could be inserted into the IgnMap with neither insertion call
knowing that there's another player with the same name. The code also
cannot rely on flecs to determine if there's another player with the
same name using World::try_lookup because this would introduce a race
condition between the try_lookup call and setting the entity name.

Author: TestingPlant

crates/hyperion/src/egress/player_join/mod.rs

@@ -1,6 +1,6 @@
 use std::{borrow::Cow, collections::BTreeSet, ops::Index};
 
-use anyhow::Context;
+use anyhow::{Context, bail};
 use flecs_ecs::prelude::*;
 use glam::DVec3;
 use hyperion_crafting::{Action, CraftingRegistry, RecipeBookState};
@@ -20,7 +20,7 @@ use valence_registry::{BiomeRegistry, RegistryCodec};
 use valence_server::entity::EntityKind;
 use valence_text::IntoText;
 
-use crate::simulation::{MovementTracking, PacketState, Pitch};
+use crate::simulation::{IgnMap, MovementTracking, PacketState, Pitch};
 
 mod list;
 pub use list::*;
@@ -43,12 +43,12 @@ use crate::{
     clippy::too_many_arguments,
     reason = "todo: we should refactor at some point"
 )]
-#[instrument(skip_all, fields(name = name))]
+#[instrument(skip_all, fields(name = &***name))]
 pub fn player_join_world(
     entity: &EntityView<'_>,
     compose: &Compose,
     uuid: uuid::Uuid,
-    name: &str,
+    name: &Name,
     io: ConnectionId,
     position: &Position,
     yaw: &Yaw,
@@ -68,6 +68,7 @@ pub fn player_join_world(
     )>,
     crafting_registry: &CraftingRegistry,
     config: &Config,
+    ign_map: &IgnMap,
 ) -> anyhow::Result<()> {
     static CACHED_DATA: once_cell::sync::OnceCell<bytes::Bytes> = once_cell::sync::OnceCell::new();
 
@@ -321,7 +322,7 @@ pub fn player_join_world(
         .add_packet(&pkt)
         .context("failed to send player list packet")?;
 
-    let player_name = vec![name];
+    let player_name = vec![&***name];
 
     compose
         .broadcast(
@@ -373,6 +374,64 @@ pub fn player_join_world(
 
     bundle.unicast(io)?;
 
+    // The player must be added to the ign map after all of its components have been set and ready
+    // to receive play packets because other threads may attempt to process the player once it is
+    // added to the ign map.
+    if let Some(previous_player) = ign_map.insert((**name).clone(), entity.id()) {
+        // Disconnect either this player or the previous player with the same username.
+        // There are some Minecraft accounts with the same username, but this is an extremely
+        // rare edge case which is not worth handling.
+        let previous_player = previous_player.entity_view(world);
+
+        let pkt = play::DisconnectS2c {
+            reason: "A different player with the same username as your account has joined on a \
+                     different device"
+                .into_cow_text(),
+        };
+
+        match previous_player.get_name() {
+            None => {
+                // previous_player must be getting processed in another thread in player_join_world
+                // because it is in ign_map but does not have a name yet. To avoid having two
+                // entities with the same name, which would cause flecs to abort, this code
+                // disconnects the current player. In the worst-case scenario, both players may get
+                // disconnected, which is okay because the players can reconnect.
+
+                warn!(
+                    "two players are simultanenously connecting with the same username '{name}'. \
+                     one player will be disconnected."
+                );
+
+                compose.unicast(&pkt, io, system)?;
+                compose.io_buf().shutdown(io, world);
+                bail!("another player with the same username is joining");
+            }
+            Some(previous_player_name) => {
+                // Kick the previous player with the same name. One player should only be able to connect
+                // to the server one time simultaneously, so if the same player connects to this server
+                // multiple times, the other connection should be disconnected. In general, this wants to
+                // disconnect the older player connection because the alternative solution of repeatedly kicking
+                // new player join attempts if an old player connection is somehow still alive would lead to bad
+                // user experience.
+                assert_eq!(previous_player_name, &***name);
+
+                warn!(
+                    "player {name} has joined with the same username of an already-connected \
+                     player. the previous player with the username will be disconnected."
+                );
+
+                previous_player.remove_name();
+
+                let previous_stream_id = previous_player.get::<&ConnectionId>(|id| *id);
+
+                compose.unicast(&pkt, previous_stream_id, system)?;
+                compose.io_buf().shutdown(previous_stream_id, world);
+            }
+        }
+    }
+
+    entity.set_name(name);
+
     info!("{name} joined the world");
 
     Ok(())
@@ -552,6 +611,7 @@ impl Module for PlayerJoinModule {
             &Compose($),
             &CraftingRegistry($),
             &Config($),
+            &IgnMap($),
             &Uuid,
             &Name,
             &Position,
@@ -570,6 +630,7 @@ impl Module for PlayerJoinModule {
                 compose,
                 crafting_registry,
                 config,
+                ign_map,
                 uuid,
                 name,
                 position,
@@ -602,6 +663,7 @@ impl Module for PlayerJoinModule {
                     &query,
                     crafting_registry,
                     config,
+                    ign_map,
                 ) {
                     warn!("player_join_world error: {e:?}");
                     compose.io_buf().shutdown(*stream_id, &world);

crates/hyperion/src/ingress/mod.rs

@@ -24,9 +24,8 @@ use crate::{
     },
     runtime::AsyncRuntime,
     simulation::{
-        AiTargetable, ChunkPosition, Comms, ConfirmBlockSequences, EntitySize, IgnMap,
-        ImmuneStatus, Name, PacketState, Pitch, Player, Position, StreamLookup, Uuid, Velocity, Xp,
-        Yaw,
+        AiTargetable, ChunkPosition, Comms, ConfirmBlockSequences, EntitySize, ImmuneStatus, Name,
+        PacketState, Pitch, Player, Position, StreamLookup, Uuid, Velocity, Xp, Yaw,
         animation::ActiveAnimation,
         blocks::Blocks,
         handlers::PacketSwitchQuery,
@@ -81,7 +80,6 @@ fn process_login(
     compose: &Compose,
     entity: &EntityView<'_>,
     system: EntityView<'_>,
-    ign_map: &IgnMap,
 ) -> anyhow::Result<()> {
     debug_assert!(
         *login_state == PacketState::Login,
@@ -151,14 +149,12 @@ fn process_login(
         .unicast(&pkt, stream_id, system)
         .context("failed to send login success packet")?;
 
-    ign_map.insert(username.clone(), entity_id, world);
-
     *login_state = PacketState::PendingPlay;
 
     world.get::<&MetadataPrefabs>(|prefabs| {
         entity
             .is_a(prefabs.player_base)
-            .set(Name::from(username))
+            .set(Name::from(username.clone()))
             .add(id::<AiTargetable>())
             .set(ImmuneStatus::default())
             .set(Uuid::from(uuid))
@@ -275,18 +271,6 @@ impl Module for IngressModule {
             }
         });
 
-        system!(
-            "update_ign_map",
-            world,
-            &mut IgnMap($),
-        )
-        .kind(id::<flecs::pipeline::OnLoad>())
-        .each_iter(|_, _, ign_map| {
-            let span = info_span!("update_ign_map");
-            let _enter = span.enter();
-            ign_map.update();
-        });
-
         system!(
             "generate_ingress_events",
             world,
@@ -422,7 +406,6 @@ impl Module for IngressModule {
             &mut hyperion_inventory::PlayerInventory,
             &mut ActiveAnimation,
             &hyperion_crafting::CraftingRegistry($),
-            &IgnMap($),
         )
         .kind(id::<flecs::pipeline::OnUpdate>())
         .each_iter(
@@ -448,7 +431,6 @@ impl Module for IngressModule {
                 inventory,
                 animation,
                 crafting_registry,
-                ign_map,
             )| {
                 let system = it.system();
                 let world = it.world();
@@ -514,7 +496,6 @@ impl Module for IngressModule {
                                 compose,
                                 &entity,
                                 system,
-                                ign_map,
                             ) {
                                 error!("failed to process login packet: {e}");
                                 let msg = format!(

crates/hyperion/src/simulation/mod.rs

@@ -1,6 +1,7 @@
 use std::{borrow::Borrow, collections::HashMap, hash::Hash, sync::Arc};
 
 use bytemuck::{Pod, Zeroable};
+use dashmap::DashMap;
 use derive_more::{Constructor, Deref, DerefMut, Display, From};
 use flecs_ecs::prelude::*;
 use geometry::aabb::Aabb;
@@ -115,7 +116,7 @@ impl<K: Eq + Hash, V> DeferredMap<K, V> {
 pub struct Name(Arc<str>);
 
 #[derive(Component, Deref, DerefMut, From, Debug, Default)]
-pub struct IgnMap(DeferredMap<Arc<str>, Entity>);
+pub struct IgnMap(DashMap<Arc<str>, Entity>);
 
 #[derive(Component, Debug, Default)]
 pub struct RaycastTravel;