From 40646eb4ba65260b347f537c1b2a4486fab58f48 Mon Sep 17 00:00:00 2001 From: ember33 Date: Wed, 18 Mar 2026 10:13:47 +0100 Subject: [PATCH] fix(spaces): wire up enforcement hooks, fix deadlocks, validate spaces - Add spawn_enforcement methods (handle_state_event_change, handle_space_child_change, handle_space_member_join) that run enforcement as background tasks to avoid recursive Send issues - Expand append_pdu hook to trigger enforcement on role events, space child changes, and space member joins - Fix deadlock risk in get_user_power_level and user_qualifies_for_room by dropping read guards before acquiring new ones - Batch room_to_space writes in populate_space with a single write lock - Add space type validation to all admin commands - Fix PL rejection check to reject any change (!=) not just lowering (<) - Fix sync_power_levels to also lower PLs for users who lost their roles Co-Authored-By: Claude Opus 4.6 (1M context) --- src/admin/space/roles.rs | 54 ++++++ src/service/rooms/roles/mod.rs | 276 ++++++++++++++++++++++++--- src/service/rooms/timeline/append.rs | 52 ++++- src/service/rooms/timeline/build.rs | 4 +- 4 files changed, 355 insertions(+), 31 deletions(-) diff --git a/src/admin/space/roles.rs b/src/admin/space/roles.rs index 09e8f12b..d5e60f69 100644 --- a/src/admin/space/roles.rs +++ b/src/admin/space/roles.rs @@ -83,6 +83,12 @@ pub enum SpaceRolesCommand { #[admin_command] async fn list(&self, space: OwnedRoomOrAliasId) -> Result { let space_id = self.services.rooms.alias.resolve(&space).await?; + if !matches!( + self.services.rooms.state_accessor.get_room_type(&space_id).await, + Ok(ruma::room::RoomType::Space) + ) { + return self.write_str("Error: The specified room is not a Space.").await; + } let roles_event_type = StateEventType::from("m.space.roles".to_owned()); let content: SpaceRolesEventContent = self @@ -119,6 +125,12 @@ async fn add( power_level: Option, ) -> Result { let space_id = self.services.rooms.alias.resolve(&space).await?; + if !matches!( + self.services.rooms.state_accessor.get_room_type(&space_id).await, + Ok(ruma::room::RoomType::Space) + ) { + return self.write_str("Error: The specified room is not a Space.").await; + } let roles_event_type = StateEventType::from("m.space.roles".to_owned()); let mut content: SpaceRolesEventContent = self @@ -159,6 +171,12 @@ async fn add( #[admin_command] async fn remove(&self, space: OwnedRoomOrAliasId, role_name: String) -> Result { let space_id = self.services.rooms.alias.resolve(&space).await?; + if !matches!( + self.services.rooms.state_accessor.get_room_type(&space_id).await, + Ok(ruma::room::RoomType::Space) + ) { + return self.write_str("Error: The specified room is not a Space.").await; + } let roles_event_type = StateEventType::from("m.space.roles".to_owned()); let mut content: SpaceRolesEventContent = self @@ -199,6 +217,12 @@ async fn assign( role_name: String, ) -> Result { let space_id = self.services.rooms.alias.resolve(&space).await?; + if !matches!( + self.services.rooms.state_accessor.get_room_type(&space_id).await, + Ok(ruma::room::RoomType::Space) + ) { + return self.write_str("Error: The specified room is not a Space.").await; + } let member_event_type = StateEventType::from("m.space.role.member".to_owned()); let mut content: SpaceRoleMemberEventContent = self @@ -243,6 +267,12 @@ async fn revoke( role_name: String, ) -> Result { let space_id = self.services.rooms.alias.resolve(&space).await?; + if !matches!( + self.services.rooms.state_accessor.get_room_type(&space_id).await, + Ok(ruma::room::RoomType::Space) + ) { + return self.write_str("Error: The specified room is not a Space.").await; + } let member_event_type = StateEventType::from("m.space.role.member".to_owned()); let mut content: SpaceRoleMemberEventContent = self @@ -288,6 +318,12 @@ async fn require( role_name: String, ) -> Result { let space_id = self.services.rooms.alias.resolve(&space).await?; + if !matches!( + self.services.rooms.state_accessor.get_room_type(&space_id).await, + Ok(ruma::room::RoomType::Space) + ) { + return self.write_str("Error: The specified room is not a Space.").await; + } let room_event_type = StateEventType::from("m.space.role.room".to_owned()); let mut content: SpaceRoleRoomEventContent = self @@ -332,6 +368,12 @@ async fn unrequire( role_name: String, ) -> Result { let space_id = self.services.rooms.alias.resolve(&space).await?; + if !matches!( + self.services.rooms.state_accessor.get_room_type(&space_id).await, + Ok(ruma::room::RoomType::Space) + ) { + return self.write_str("Error: The specified room is not a Space.").await; + } let room_event_type = StateEventType::from("m.space.role.room".to_owned()); let mut content: SpaceRoleRoomEventContent = self @@ -372,6 +414,12 @@ async fn unrequire( #[admin_command] async fn user(&self, space: OwnedRoomOrAliasId, user_id: OwnedUserId) -> Result { let space_id = self.services.rooms.alias.resolve(&space).await?; + if !matches!( + self.services.rooms.state_accessor.get_room_type(&space_id).await, + Ok(ruma::room::RoomType::Space) + ) { + return self.write_str("Error: The specified room is not a Space.").await; + } let user_roles = self.services.rooms.roles.user_roles.read().await; let roles = user_roles @@ -401,6 +449,12 @@ async fn user(&self, space: OwnedRoomOrAliasId, user_id: OwnedUserId) -> Result #[admin_command] async fn room(&self, space: OwnedRoomOrAliasId, room_id: OwnedRoomId) -> Result { let space_id = self.services.rooms.alias.resolve(&space).await?; + if !matches!( + self.services.rooms.state_accessor.get_room_type(&space_id).await, + Ok(ruma::room::RoomType::Space) + ) { + return self.write_str("Error: The specified room is not a Space.").await; + } let room_reqs = self.services.rooms.roles.room_requirements.read().await; let requirements = room_reqs diff --git a/src/service/rooms/roles/mod.rs b/src/service/rooms/roles/mod.rs index ae8d40f1..19c1364a 100644 --- a/src/service/rooms/roles/mod.rs +++ b/src/service/rooms/roles/mod.rs @@ -303,6 +303,8 @@ pub async fn populate_space(&self, space_id: &RoomId) { .insert(space_id.to_owned(), room_reqs_map); // 4. Read all m.space.child state events → build room_to_space reverse index + let mut child_rooms: Vec = Vec::new(); + self.services .state_accessor .state_keys_with_ids(shortstatehash, &StateEventType::SpaceChild) @@ -316,7 +318,6 @@ pub async fn populate_space(&self, space_id: &RoomId) { .await }) .ready_filter_map(|(state_key, pdu)| { - // Only index children that have a valid via list if let Ok(content) = pdu.get_content::() { if content.via.is_empty() { return None; @@ -328,15 +329,18 @@ pub async fn populate_space(&self, space_id: &RoomId) { Some(child_room_id) }) .for_each(|child_room_id| { - let space_owned = space_id.to_owned(); - async move { - self.room_to_space - .write() - .await - .insert(child_room_id, space_owned); - } + child_rooms.push(child_room_id); + async {} }) .await; + + // Single write lock for all children + { + let mut room_to_space = self.room_to_space.write().await; + for child_room_id in child_rooms { + room_to_space.insert(child_room_id, space_id.to_owned()); + } + } } } @@ -348,10 +352,14 @@ pub async fn get_user_power_level( space_id: &RoomId, user_id: &UserId, ) -> Option { - let roles_map = self.roles.read().await; - let user_roles_map = self.user_roles.read().await; - let role_defs = roles_map.get(space_id)?; - let user_assigned = user_roles_map.get(space_id)?.get(user_id)?; + let role_defs = { + let guard = self.roles.read().await; + guard.get(space_id).cloned()? + }; + let user_assigned = { + let guard = self.user_roles.read().await; + guard.get(space_id)?.get(user_id).cloned()? + }; user_assigned .iter() .filter_map(|role_name| role_defs.get(role_name)?.power_level) @@ -366,22 +374,28 @@ pub async fn user_qualifies_for_room( room_id: &RoomId, user_id: &UserId, ) -> bool { - let reqs = self.room_requirements.read().await; - let Some(space_reqs) = reqs.get(space_id) else { - return true; + let required = { + let guard = self.room_requirements.read().await; + let Some(space_reqs) = guard.get(space_id) else { + return true; + }; + let Some(required) = space_reqs.get(room_id) else { + return true; + }; + if required.is_empty() { + return true; + } + required.clone() }; - let Some(required) = space_reqs.get(room_id) else { - return true; - }; - if required.is_empty() { - return true; - } - let user_map = self.user_roles.read().await; - let Some(space_users) = user_map.get(space_id) else { - return false; - }; - let Some(user_assigned) = space_users.get(user_id) else { - return false; + let user_assigned = { + let guard = self.user_roles.read().await; + let Some(space_users) = guard.get(space_id) else { + return false; + }; + let Some(assigned) = space_users.get(user_id) else { + return false; + }; + assigned.clone() }; required.iter().all(|r| user_assigned.contains(r)) } @@ -435,6 +449,14 @@ pub async fn sync_power_levels(&self, space_id: &RoomId, room_id: &RoomId) -> Re .insert(user_id.clone(), space_pl_int); changed = true; } + } else { + // User has no space role PL — ensure they're at default + if let Some(current) = power_levels_content.users.get(user_id) { + if *current != power_levels_content.users_default { + power_levels_content.users.remove(user_id); + changed = true; + } + } } } @@ -549,6 +571,206 @@ pub async fn auto_join_qualifying_rooms( Ok(()) } +/// Handle a state event change that may require enforcement. +/// Spawns enforcement as a background task to avoid recursive Send issues +/// in the append_pdu path. +impl Service { + pub fn handle_state_event_change( + self: &Arc, + space_id: OwnedRoomId, + event_type: String, + state_key: String, + ) { + if !self.is_enabled() { + return; + } + + let this = Arc::clone(self); + self.server.runtime().spawn(async move { + // Always repopulate cache first + this.populate_space(&space_id).await; + + match event_type.as_str() { + | "m.space.roles" => { + // Role definitions changed — sync PLs in all child rooms + let child_rooms: Vec = this + .room_to_space + .read() + .await + .iter() + .filter(|(_, parent)| **parent == *space_id) + .map(|(child, _)| child.clone()) + .collect(); + for child_room_id in &child_rooms { + if let Err(e) = + this.sync_power_levels(&space_id, child_room_id).await + { + debug_warn!("Failed to sync PLs in {child_room_id}: {e}"); + } + } + }, + | "m.space.role.member" => { + // User's roles changed — auto-join/kick + PL sync + if let Ok(user_id) = UserId::parse(state_key.as_str()) { + if let Err(e) = + this.auto_join_qualifying_rooms(&space_id, &user_id).await + { + debug_warn!( + "Space role auto-join failed for {user_id}: {e}" + ); + } + if let Err(e) = + this.kick_unqualified_from_rooms(&space_id, &user_id).await + { + debug_warn!( + "Space role auto-kick failed for {user_id}: {e}" + ); + } + // Sync power levels in all child rooms + let child_rooms: Vec = this + .room_to_space + .read() + .await + .iter() + .filter(|(_, parent)| **parent == *space_id) + .map(|(child, _)| child.clone()) + .collect(); + for child_room_id in &child_rooms { + if let Err(e) = + this.sync_power_levels(&space_id, child_room_id).await + { + debug_warn!( + "Failed to sync PLs in {child_room_id}: {e}" + ); + } + } + } + }, + | "m.space.role.room" => { + // Room requirements changed — kick unqualified members + if let Ok(target_room) = RoomId::parse(state_key.as_str()) { + let members: Vec = this + .services + .state_cache + .room_members(&target_room) + .map(ToOwned::to_owned) + .collect() + .await; + for member in &members { + if !this + .user_qualifies_for_room( + &space_id, + &target_room, + member, + ) + .await + { + if let Err(e) = this + .kick_unqualified_from_rooms(&space_id, member) + .await + { + debug_warn!( + "Space role requirement kick failed for {member}: {e}" + ); + } + } + } + } + }, + | _ => {}, + } + }); + } + + /// Handle a new m.space.child event — update index and auto-join qualifying + /// members. + pub fn handle_space_child_change( + self: &Arc, + space_id: OwnedRoomId, + child_room_id: OwnedRoomId, + ) { + if !self.is_enabled() { + return; + } + + let this = Arc::clone(self); + self.server.runtime().spawn(async move { + // Update the reverse index + this.room_to_space + .write() + .await + .insert(child_room_id.clone(), space_id.clone()); + + // Auto-join all qualifying space members + let space_members: Vec = this + .services + .state_cache + .room_members(&space_id) + .map(ToOwned::to_owned) + .collect() + .await; + + for member in &space_members { + if this + .user_qualifies_for_room(&space_id, &child_room_id, member) + .await + { + if !this + .services + .state_cache + .is_joined(member, &child_room_id) + .await + { + if let Err(e) = + this.auto_join_qualifying_rooms(&space_id, member).await + { + debug_warn!( + "Auto-join failed for {member} on new child room: {e}" + ); + } + } + } + } + }); + } + + /// Handle a user joining a Space — auto-join them to qualifying child rooms. + pub fn handle_space_member_join( + self: &Arc, + space_id: OwnedRoomId, + user_id: OwnedUserId, + ) { + if !self.is_enabled() { + return; + } + + let this = Arc::clone(self); + self.server.runtime().spawn(async move { + if let Err(e) = this.auto_join_qualifying_rooms(&space_id, &user_id).await { + debug_warn!("Auto-join on Space join failed for {user_id}: {e}"); + } + // Also sync their power levels + let child_rooms: Vec = this + .room_to_space + .read() + .await + .iter() + .filter(|(_, parent)| **parent == *space_id) + .map(|(child, _)| child.clone()) + .collect(); + for child_room_id in &child_rooms { + if let Err(e) = + this.sync_power_levels(&space_id, &child_room_id).await + { + debug_warn!( + "Failed to sync PLs on join for {child_room_id}: {e}" + ); + } + } + }); + } +} + /// Remove a user from all child rooms they no longer qualify for. /// /// Iterates over child rooms that have role requirements for the given diff --git a/src/service/rooms/timeline/append.rs b/src/service/rooms/timeline/append.rs index 3b07bdea..74e3d251 100644 --- a/src/service/rooms/timeline/append.rs +++ b/src/service/rooms/timeline/append.rs @@ -361,15 +361,63 @@ where // Space permission cascading: react to role-related state events if self.services.roles.is_enabled() { - if let Some(_state_key) = pdu.state_key() { + if let Some(state_key) = pdu.state_key() { let event_type_str = pdu.event_type().to_string(); match event_type_str.as_str() { | "m.space.roles" | "m.space.role.member" | "m.space.role.room" => { - self.services.roles.populate_space(room_id).await; + let roles: Arc = + Arc::clone(&*self.services.roles); + roles.handle_state_event_change( + room_id.to_owned(), + event_type_str, + state_key.to_string(), + ); }, | _ => {}, } } + // Handle m.space.child changes + if *pdu.kind() == TimelineEventType::SpaceChild { + if let Some(state_key) = pdu.state_key() { + if let Ok(child_room_id) = ruma::RoomId::parse(state_key) { + let roles: Arc = + Arc::clone(&*self.services.roles); + roles.handle_space_child_change( + room_id.to_owned(), + child_room_id.to_owned(), + ); + } + } + } + // Handle m.room.member join in a Space — auto-join child rooms + if *pdu.kind() == TimelineEventType::RoomMember { + if let Some(state_key) = pdu.state_key() { + if let Ok(content) = + pdu.get_content::() + { + if content.membership + == ruma::events::room::member::MembershipState::Join + { + if let Ok(user_id) = UserId::parse(state_key) { + if matches!( + self.services + .state_accessor + .get_room_type(room_id) + .await, + Ok(ruma::room::RoomType::Space) + ) { + let roles: Arc = + Arc::clone(&*self.services.roles); + roles.handle_space_member_join( + room_id.to_owned(), + user_id.to_owned(), + ); + } + } + } + } + } + } } // CONCERN: If we receive events with a relation out-of-order, we never write diff --git a/src/service/rooms/timeline/build.rs b/src/service/rooms/timeline/build.rs index 15a44d90..9af5d5ce 100644 --- a/src/service/rooms/timeline/build.rs +++ b/src/service/rooms/timeline/build.rs @@ -110,9 +110,9 @@ pub async fn build_and_append_pdu( if let Some(space_pl) = self.services.roles.get_user_power_level(&parent_space, user_id).await { - if i64::from(*proposed_pl) < space_pl { + if i64::from(*proposed_pl) != space_pl { return Err!(Request(Forbidden( - "Cannot set power level below Space-granted level" + "Cannot change power level that is set by Space roles" ))); } }