Merge branch 'crabman/group-fixes' into 'master'

Fix group chat leak

See merge request veloren/veloren!4102

CHANGELOG.md

@@ -58,6 +58,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fixed wild roaming cyclop loot table to not drop the quarry key
 - Dungeons now have an outer wall, preventing them from intersecting with caves or leaving holes in sides of mountains.
 - Location names are displayed in character selection dialog
+- You can no longer write messages to old groups after being kicked and not having updated your chat mode.
 
 ## [0.15.0] - 2023-07-01
 

common/src/comp/chat.rs

@@ -18,7 +18,7 @@ pub enum ChatMode {
     /// Talk to players in your region of the world
     Region,
     /// Talk to your current group of players
-    Group(Group),
+    Group,
     /// Talk to your faction
     Faction(String),
     /// Talk to every player on the server
@@ -31,19 +31,31 @@ impl Component for ChatMode {
 
 impl ChatMode {
     /// Create a plain message from your current chat mode and uuid.
-    pub fn to_plain_msg(&self, from: Uid, text: impl ToString) -> UnresolvedChatMsg {
+    pub fn to_plain_msg(
+        &self,
+        from: Uid,
+        text: impl ToString,
+        group: Option<Group>,
+    ) -> Result<UnresolvedChatMsg, &'static str> {
         let chat_type = match self {
             ChatMode::Tell(to) => ChatType::Tell(from, *to),
             ChatMode::Say => ChatType::Say(from),
             ChatMode::Region => ChatType::Region(from),
-            ChatMode::Group(group) => ChatType::Group(from, *group),
+            ChatMode::Group => ChatType::Group(
+                from,
+                group.ok_or(
+                    "You tried sending a group message while not belonging to a group. Use /world \
+                     or /region to change your chat mode.",
+                )?,
+            ),
             ChatMode::Faction(faction) => ChatType::Faction(from, faction.clone()),
             ChatMode::World => ChatType::World(from),
         };
-        UnresolvedChatMsg {
+
+        Ok(UnresolvedChatMsg {
             chat_type,
             content: Content::Plain(text.to_string()),
-        }
+        })
     }
 }
 

server/src/cmd.rs

@@ -2888,7 +2888,9 @@ fn handle_tell(
         } else {
             message_opt.join(" ")
         };
-        server.state.send_chat(mode.to_plain_msg(target_uid, msg));
+        server
+            .state
+            .send_chat(mode.to_plain_msg(target_uid, msg, None)?);
         server.notify_client(target, ServerGeneral::ChatMode(mode));
         Ok(())
     } else {
@@ -2913,7 +2915,7 @@ fn handle_faction(
         let msg = args.join(" ");
         if !msg.is_empty() {
             if let Some(uid) = server.state.ecs().read_storage().get(target) {
-                server.state.send_chat(mode.to_plain_msg(*uid, msg));
+                server.state.send_chat(mode.to_plain_msg(*uid, msg, None)?);
             }
         }
         server.notify_client(target, ServerGeneral::ChatMode(mode));
@@ -2933,14 +2935,16 @@ fn handle_group(
     no_sudo(client, target)?;
 
     let groups = server.state.ecs().read_storage::<comp::Group>();
-    if let Some(group) = groups.get(target) {
-        let mode = comp::ChatMode::Group(*group);
+    if let Some(group) = groups.get(target).copied() {
+        let mode = comp::ChatMode::Group;
         drop(groups);
         insert_or_replace_component(server, target, mode.clone(), "target")?;
         let msg = args.join(" ");
         if !msg.is_empty() {
             if let Some(uid) = server.state.ecs().read_storage().get(target) {
-                server.state.send_chat(mode.to_plain_msg(*uid, msg));
+                server
+                    .state
+                    .send_chat(mode.to_plain_msg(*uid, msg, Some(group))?);
             }
         }
         server.notify_client(target, ServerGeneral::ChatMode(mode));
@@ -3064,7 +3068,7 @@ fn handle_region(
     let msg = args.join(" ");
     if !msg.is_empty() {
         if let Some(uid) = server.state.ecs().read_storage().get(target) {
-            server.state.send_chat(mode.to_plain_msg(*uid, msg));
+            server.state.send_chat(mode.to_plain_msg(*uid, msg, None)?);
         }
     }
     server.notify_client(target, ServerGeneral::ChatMode(mode));
@@ -3085,7 +3089,7 @@ fn handle_say(
     let msg = args.join(" ");
     if !msg.is_empty() {
         if let Some(uid) = server.state.ecs().read_storage().get(target) {
-            server.state.send_chat(mode.to_plain_msg(*uid, msg));
+            server.state.send_chat(mode.to_plain_msg(*uid, msg, None)?);
         }
     }
     server.notify_client(target, ServerGeneral::ChatMode(mode));
@@ -3106,7 +3110,7 @@ fn handle_world(
     let msg = args.join(" ");
     if !msg.is_empty() {
         if let Some(uid) = server.state.ecs().read_storage().get(target) {
-            server.state.send_chat(mode.to_plain_msg(*uid, msg));
+            server.state.send_chat(mode.to_plain_msg(*uid, msg, None)?);
         }
     }
     server.notify_client(target, ServerGeneral::ChatMode(mode));

server/src/state_ext.rs

@@ -1076,22 +1076,26 @@ impl StateExt for State {
                 },
                 comp::ChatType::Group(from, g) => {
                     if group_info.is_none() {
-                        // group not found, reply with command error
+                        // Group not found, reply with command error
+                        // This should usually NEVER happen since now it is checked whether the
+                        // sender is still in the group upon emitting the message (TODO: Can this be
+                        // triggered if the message is sent in the same tick as the sender is
+                        // removed from the group?)
+
                         let reply = comp::ChatType::CommandError.into_plain_msg(
                             "You are using group chat but do not belong to a group. Use /world or \
                              /region to change chat.",
                         );
 
-                        if let Some((client, _)) =
-                            (&ecs.read_storage::<Client>(), &ecs.read_storage::<Uid>())
-                                .join()
-                                .find(|(_, uid)| *uid == from)
+                        let clients = ecs.read_storage::<Client>();
+                        if let Some(client) =
+                            entity_from_uid(*from).and_then(|entity| clients.get(entity))
                         {
                             client.send_fallible(ServerGeneral::ChatMsg(reply));
                         }
-                        return;
+                    } else {
+                        send_to_group(g, ecs, &resolved_msg);
                     }
-                    send_to_group(g, ecs, &resolved_msg);
                 },
                 comp::ChatType::GroupMeta(g) => {
                     send_to_group(g, ecs, &resolved_msg);

server/src/sys/msg/general.rs

@@ -1,12 +1,12 @@
 use crate::client::Client;
 use common::{
-    comp::{ChatMode, Player},
+    comp::{ChatMode, ChatType, Group, Player},
     event::{EventBus, ServerEvent},
     resources::Time,
     uid::Uid,
 };
 use common_ecs::{Job, Origin, Phase, System};
-use common_net::msg::ClientGeneral;
+use common_net::msg::{ClientGeneral, ServerGeneral};
 use rayon::prelude::*;
 use specs::{Entities, Join, ParJoin, Read, ReadStorage, WriteStorage};
 use tracing::{debug, error, warn};
@@ -15,10 +15,11 @@ impl Sys {
     fn handle_general_msg(
         server_emitter: &mut common::event::Emitter<'_, ServerEvent>,
         entity: specs::Entity,
-        _client: &Client,
+        client: &Client,
         player: Option<&Player>,
         uids: &ReadStorage<'_, Uid>,
         chat_modes: &ReadStorage<'_, ChatMode>,
+        groups: &ReadStorage<'_, Group>,
         msg: ClientGeneral,
     ) -> Result<(), crate::error::Error> {
         match msg {
@@ -27,8 +28,17 @@ impl Sys {
                     if let Some(from) = uids.get(entity) {
                         const CHAT_MODE_DEFAULT: &ChatMode = &ChatMode::default();
                         let mode = chat_modes.get(entity).unwrap_or(CHAT_MODE_DEFAULT);
-                        // Send chat message
-                        server_emitter.emit(ServerEvent::Chat(mode.to_plain_msg(*from, message)));
+                        // Try sending the chat message
+                        match mode.to_plain_msg(*from, message, groups.get(entity).copied()) {
+                            Ok(message) => {
+                                server_emitter.emit(ServerEvent::Chat(message));
+                            },
+                            Err(error) => {
+                                client.send_fallible(ServerGeneral::ChatMsg(
+                                    ChatType::CommandError.into_plain_msg(error),
+                                ));
+                            },
+                        }
                     } else {
                         error!("Could not send message. Missing player uid");
                     }
@@ -71,6 +81,7 @@ impl<'a> System<'a> for Sys {
         ReadStorage<'a, Uid>,
         ReadStorage<'a, ChatMode>,
         ReadStorage<'a, Player>,
+        ReadStorage<'a, Group>,
         WriteStorage<'a, Client>,
     );
 
@@ -80,7 +91,7 @@ impl<'a> System<'a> for Sys {
 
     fn run(
         _job: &mut Job<Self>,
-        (entities, server_event_bus, time, uids, chat_modes, players, mut clients): Self::SystemData,
+        (entities, server_event_bus, time, uids, chat_modes, players, groups, mut clients): Self::SystemData,
     ) {
         (&entities, &mut clients, players.maybe())
             .par_join()
@@ -95,6 +106,7 @@ impl<'a> System<'a> for Sys {
                             player,
                             &uids,
                             &chat_modes,
+                            &groups,
                             msg,
                         )
                     });

voxygen/src/hud/chat.rs

@@ -801,7 +801,7 @@ fn render_chat_mode(chat_mode: &ChatMode, imgs: &Imgs) -> (Color, conrod_core::i
         ChatMode::Say => (SAY_COLOR, imgs.chat_say_small),
         ChatMode::Region => (REGION_COLOR, imgs.chat_region_small),
         ChatMode::Faction(_) => (FACTION_COLOR, imgs.chat_faction_small),
-        ChatMode::Group(_) => (GROUP_COLOR, imgs.chat_group_small),
+        ChatMode::Group => (GROUP_COLOR, imgs.chat_group_small),
         ChatMode::Tell(_) => (TELL_COLOR, imgs.chat_tell_small),
     }
 }