From dd1e9f09793658e1b94dfe6d924b2022fcf3545a Mon Sep 17 00:00:00 2001 From: ember33 Date: Wed, 18 Mar 2026 11:53:11 +0100 Subject: [PATCH] refactor(spaces): fix clippy, extract testable functions, improve test coverage Fix all clippy warnings in space roles files: dangerous `as` casts, `to_string()` on &str, format string inlining, items-after-statements, needless borrows, large futures, semicolons outside blocks, and let-else patterns. Extract `compute_user_power_level` and `roles_satisfy_requirements` as pure free functions so the core logic can be unit-tested without async service dependencies. Update all tests in tests.rs and integration_tests.rs to call the real extracted functions instead of reimplementing the logic inline. Add negative deserialization tests for RoleDefinition, SpaceRoleMemberEventContent, and SpaceRoleRoomEventContent. Improve doc comments on handle_* methods and add module-level documentation to cache_tests.rs. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/admin/space/roles.rs | 10 +- src/core/matrix/space_roles.rs | 18 ++++ src/service/rooms/roles/cache_tests.rs | 6 +- src/service/rooms/roles/integration_tests.rs | 47 ++++----- src/service/rooms/roles/mod.rs | 103 +++++++++++-------- src/service/rooms/roles/tests.rs | 48 +++------ src/service/rooms/timeline/append.rs | 2 +- src/service/rooms/timeline/build.rs | 6 +- 8 files changed, 126 insertions(+), 114 deletions(-) diff --git a/src/admin/space/roles.rs b/src/admin/space/roles.rs index b29ff20c..ad14e5dd 100644 --- a/src/admin/space/roles.rs +++ b/src/admin/space/roles.rs @@ -1,3 +1,5 @@ +use std::fmt::Write; + use clap::Subcommand; use conduwuit::{Err, Event, Result}; use conduwuit_core::matrix::space_roles::{ @@ -134,13 +136,13 @@ async fn list(&self, space: OwnedRoomOrAliasId) -> Result { return self.write_str("No roles defined in this space.").await; } - let mut msg = format!("Roles in {}:\n```\n", space_id); + let mut msg = format!("Roles in {space_id}:\n```\n"); for (name, def) in &content.roles { let pl = def .power_level .map(|p| format!(" (power_level: {p})")) .unwrap_or_default(); - msg.push_str(&format!("- {name}: {}{pl}\n", def.description)); + let _ = writeln!(msg, "- {name}: {}{pl}", def.description); } msg.push_str("```"); @@ -332,7 +334,7 @@ async fn assign( if !role_defs.roles.contains_key(&role_name) { return self - .write_str(&format!("Error: Role '{}' does not exist in this space.", role_name)) + .write_str(&format!("Error: Role '{role_name}' does not exist in this space.")) .await; } @@ -438,7 +440,7 @@ async fn require( if !role_defs.roles.contains_key(&role_name) { return self - .write_str(&format!("Error: Role '{}' does not exist in this space.", role_name)) + .write_str(&format!("Error: Role '{role_name}' does not exist in this space.")) .await; } diff --git a/src/core/matrix/space_roles.rs b/src/core/matrix/space_roles.rs index 9befb133..054126ad 100644 --- a/src/core/matrix/space_roles.rs +++ b/src/core/matrix/space_roles.rs @@ -181,4 +181,22 @@ mod tests { let content: SpaceRoleMemberEventContent = serde_json::from_str(json).unwrap(); assert_eq!(content.roles, vec!["nsfw"]); } + + #[test] + fn missing_description_fails() { + let json = r#"{"power_level":100}"#; + assert!(serde_json::from_str::(json).is_err()); + } + + #[test] + fn wrong_type_for_roles_fails() { + let json = r#"{"roles":"not_an_array"}"#; + assert!(serde_json::from_str::(json).is_err()); + } + + #[test] + fn wrong_type_for_required_roles_fails() { + let json = r#"{"required_roles":42}"#; + assert!(serde_json::from_str::(json).is_err()); + } } diff --git a/src/service/rooms/roles/cache_tests.rs b/src/service/rooms/roles/cache_tests.rs index a37c302f..bb869e20 100644 --- a/src/service/rooms/roles/cache_tests.rs +++ b/src/service/rooms/roles/cache_tests.rs @@ -1,4 +1,8 @@ -//! Tests for cache consistency of the space roles index structures. +//! Cache consistency tests using a MockCache that mirrors the Service's +//! cache structures. These tests validate the algorithm/logic but do NOT +//! exercise the actual Service methods (which require async service +//! dependencies). See `tests.rs` for tests that call the extracted pure +//! logic functions directly. use std::collections::{BTreeMap, HashMap, HashSet}; diff --git a/src/service/rooms/roles/integration_tests.rs b/src/service/rooms/roles/integration_tests.rs index 15179ebc..d5f8f3d2 100644 --- a/src/service/rooms/roles/integration_tests.rs +++ b/src/service/rooms/roles/integration_tests.rs @@ -2,6 +2,7 @@ use std::collections::{HashMap, HashSet}; use ruma::{room_id, user_id}; +use super::{compute_user_power_level, roles_satisfy_requirements}; use super::tests::{make_requirements, make_roles, make_user_roles}; #[test] @@ -9,13 +10,13 @@ fn scenario_user_gains_and_loses_access() { let room_reqs = make_requirements(&["nsfw"]); let no_roles: HashSet = HashSet::new(); - assert!(!room_reqs.iter().all(|r| no_roles.contains(r))); + assert!(!roles_satisfy_requirements(&room_reqs, &no_roles)); let with_nsfw = make_user_roles(&["nsfw"]); - assert!(room_reqs.iter().all(|r| with_nsfw.contains(r))); + assert!(roles_satisfy_requirements(&room_reqs, &with_nsfw)); let no_roles: HashSet = HashSet::new(); - assert!(!room_reqs.iter().all(|r| no_roles.contains(r))); + assert!(!roles_satisfy_requirements(&room_reqs, &no_roles)); } #[test] @@ -24,12 +25,12 @@ fn scenario_room_adds_requirement_existing_members_checked() { let bob_roles = make_user_roles(&["vip", "nsfw"]); let empty_reqs: HashSet = HashSet::new(); - assert!(empty_reqs.iter().all(|r| alice_roles.contains(r))); - assert!(empty_reqs.iter().all(|r| bob_roles.contains(r))); + assert!(roles_satisfy_requirements(&empty_reqs, &alice_roles)); + assert!(roles_satisfy_requirements(&empty_reqs, &bob_roles)); let new_reqs = make_requirements(&["nsfw"]); - assert!(!new_reqs.iter().all(|r| alice_roles.contains(r))); - assert!(new_reqs.iter().all(|r| bob_roles.contains(r))); + assert!(!roles_satisfy_requirements(&new_reqs, &alice_roles)); + assert!(roles_satisfy_requirements(&new_reqs, &bob_roles)); } #[test] @@ -41,13 +42,13 @@ fn scenario_multiple_rooms_different_requirements() { let vip_reqs = make_requirements(&["vip"]); let both_reqs = make_requirements(&["nsfw", "vip"]); - assert!(nsfw_reqs.iter().all(|r| alice_roles.contains(r))); - assert!(vip_reqs.iter().all(|r| alice_roles.contains(r))); - assert!(both_reqs.iter().all(|r| alice_roles.contains(r))); + assert!(roles_satisfy_requirements(&nsfw_reqs, &alice_roles)); + assert!(roles_satisfy_requirements(&vip_reqs, &alice_roles)); + assert!(roles_satisfy_requirements(&both_reqs, &alice_roles)); - assert!(nsfw_reqs.iter().all(|r| bob_roles.contains(r))); - assert!(!vip_reqs.iter().all(|r| bob_roles.contains(r))); - assert!(!both_reqs.iter().all(|r| bob_roles.contains(r))); + assert!(roles_satisfy_requirements(&nsfw_reqs, &bob_roles)); + assert!(!roles_satisfy_requirements(&vip_reqs, &bob_roles)); + assert!(!roles_satisfy_requirements(&both_reqs, &bob_roles)); } #[test] @@ -59,22 +60,10 @@ fn scenario_power_level_cascading_highest_wins() { ]); let admin_mod = make_user_roles(&["admin", "mod"]); - assert_eq!( - admin_mod - .iter() - .filter_map(|r| roles.get(r)?.power_level) - .max(), - Some(100) - ); + assert_eq!(compute_user_power_level(&roles, &admin_mod), Some(100)); let helper = make_user_roles(&["helper"]); - assert_eq!( - helper - .iter() - .filter_map(|r| roles.get(r)?.power_level) - .max(), - Some(25) - ); + assert_eq!(compute_user_power_level(&roles, &helper), Some(25)); } #[test] @@ -107,7 +96,7 @@ fn scenario_identify_auto_join_candidates() { let qualifying: Vec<_> = room_reqs .iter() - .filter(|(_, reqs)| reqs.iter().all(|r| alice_roles.contains(r))) + .filter(|(_, reqs)| roles_satisfy_requirements(reqs, &alice_roles)) .map(|(name, _)| name.clone()) .collect(); @@ -132,7 +121,7 @@ fn scenario_identify_kick_candidates_after_role_revocation() { let kick_from: Vec<_> = rooms .iter() - .filter(|(_, reqs)| !reqs.iter().all(|r| alice_roles_after.contains(r))) + .filter(|(_, reqs)| !roles_satisfy_requirements(reqs, &alice_roles_after)) .map(|(name, _)| name.clone()) .collect(); diff --git a/src/service/rooms/roles/mod.rs b/src/service/rooms/roles/mod.rs index 5698134f..11925a58 100644 --- a/src/service/rooms/roles/mod.rs +++ b/src/service/rooms/roles/mod.rs @@ -150,7 +150,7 @@ impl crate::Service for Service { | Ok(RoomType::Space) => { debug!("Populating space roles cache for {room_id}"); self.populate_space(room_id).await; - space_count += 1; + space_count = space_count.saturating_add(1); }, | _ => continue, } @@ -241,7 +241,7 @@ pub async fn populate_space(&self, space_id: &RoomId) { } // Check cache capacity — if over limit, clear and let spaces repopulate on demand - if self.roles.read().await.len() >= self.server.config.space_roles_cache_capacity as usize { + if self.roles.read().await.len() >= usize::try_from(self.server.config.space_roles_cache_capacity).unwrap_or(usize::MAX) { self.roles.write().await.clear(); self.user_roles.write().await.clear(); self.room_requirements.write().await.clear(); @@ -376,8 +376,8 @@ pub async fn populate_space(&self, space_id: &RoomId) { // Update forward index { let mut space_to_rooms = self.space_to_rooms.write().await; - space_to_rooms.insert(space_id.to_owned(), child_rooms.iter().cloned().collect()); - } + space_to_rooms.insert(space_id.to_owned(), child_rooms.iter().cloned().collect()) + }; // Single write lock for all children — clear stale entries first { @@ -398,6 +398,27 @@ pub async fn populate_space(&self, space_id: &RoomId) { } } +/// Compute the maximum power level from a user's assigned roles. +#[must_use] +pub fn compute_user_power_level( + role_defs: &BTreeMap, + assigned: &HashSet, +) -> Option { + assigned + .iter() + .filter_map(|role_name| role_defs.get(role_name)?.power_level) + .max() +} + +/// Check if a set of assigned roles satisfies all requirements. +#[must_use] +pub fn roles_satisfy_requirements( + required: &HashSet, + assigned: &HashSet, +) -> bool { + required.iter().all(|r| assigned.contains(r)) +} + /// Get a user's effective power level from Space roles. /// Returns None if user has no roles with power levels. #[implement(Service)] @@ -406,18 +427,9 @@ pub async fn get_user_power_level( space_id: &RoomId, user_id: &UserId, ) -> Option { - 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) - .max() + let role_defs = { self.roles.read().await.get(space_id).cloned()? }; + let user_assigned = { self.user_roles.read().await.get(space_id)?.get(user_id).cloned()? }; + compute_user_power_level(&role_defs, &user_assigned) } /// Check if a user has all required roles for a room. @@ -451,7 +463,7 @@ pub async fn user_qualifies_for_room( }; assigned.clone() }; - required.iter().all(|r| user_assigned.contains(r)) + roles_satisfy_requirements(&required, &user_assigned) } /// Get the parent Spaces of a child room, if any. @@ -681,8 +693,11 @@ pub async fn auto_join_qualifying_rooms( } /// 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. +/// +/// Spawns a background task (gated by the enforcement semaphore) to +/// repopulate the cache and trigger enforcement actions based on the +/// event type. Deduplicated per-space to avoid redundant work during +/// bulk operations. impl Service { pub fn handle_state_event_change( self: &Arc, @@ -703,8 +718,8 @@ impl Service { if pending.contains(&space_id) { return; } - pending.insert(space_id.clone()); - } + pending.insert(space_id.clone()) + }; let _permit = this.enforcement_semaphore.acquire().await; @@ -732,7 +747,7 @@ impl Service { .await; for member in &space_members { if let Err(e) = - this.kick_unqualified_from_rooms(&space_id, member).await + Box::pin(this.kick_unqualified_from_rooms(&space_id, member)).await { debug_warn!( "Role definition revalidation kick failed for {member}: {e}" @@ -744,14 +759,14 @@ impl Service { // 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 + 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 + Box::pin(this.kick_unqualified_from_rooms(&space_id, user_id)).await { debug_warn!( "Space role auto-kick failed for {user_id}: {e}" @@ -776,7 +791,7 @@ impl Service { let members: Vec = this .services .state_cache - .room_members(&target_room) + .room_members(target_room) .map(ToOwned::to_owned) .collect() .await; @@ -784,13 +799,13 @@ impl Service { if !this .user_qualifies_for_room( &space_id, - &target_room, + target_room, member, ) .await { - if let Err(e) = this - .kick_unqualified_from_rooms(&space_id, member) + if let Err(e) = Box::pin(this + .kick_unqualified_from_rooms(&space_id, member)) .await { debug_warn!( @@ -809,8 +824,12 @@ impl Service { }); } - /// Handle a new m.space.child event — update index and auto-join qualifying - /// members, or remove child from index if `via` is empty. + /// Handle a new `m.space.child` event — update index and auto-join + /// qualifying members. + /// + /// If the child event's `via` field is empty the child is removed from + /// both the forward and reverse indexes. Otherwise the child is added + /// and all qualifying space members are auto-joined. pub fn handle_space_child_change( self: &Arc, space_id: OwnedRoomId, @@ -961,7 +980,12 @@ impl Service { }); } - /// Handle a user joining a Space — auto-join them to qualifying child rooms. + /// Handle a user joining a Space — auto-join them to qualifying child + /// rooms. + /// + /// Spawns a background task that auto-joins the user into every child + /// room they qualify for, then synchronizes their power levels across + /// all child rooms. pub fn handle_space_member_join( self: &Arc, space_id: OwnedRoomId, @@ -987,7 +1011,7 @@ impl Service { let child_rooms = this.get_child_rooms(&space_id).await; for child_room_id in &child_rooms { if let Err(e) = - this.sync_power_levels(&space_id, &child_room_id).await + this.sync_power_levels(&space_id, child_room_id).await { debug_warn!( "Failed to sync PLs on join for {child_room_id}: {e}" @@ -1059,19 +1083,16 @@ pub async fn kick_unqualified_from_rooms( } // Get existing member event content for the kick - let member_content = match self + let Ok(member_content) = self .services .state_accessor .get_member(child_room_id, user_id) .await - { - | Ok(event) => event, - | Err(_) => { - debug_warn!( - "Could not get member event for {user_id} in {child_room_id}, skipping kick" - ); - continue; - }, + else { + debug_warn!( + "Could not get member event for {user_id} in {child_room_id}, skipping kick" + ); + continue; }; let state_lock = self.services.state.mutex.lock(child_room_id).await; diff --git a/src/service/rooms/roles/tests.rs b/src/service/rooms/roles/tests.rs index 471ec581..a9b0068b 100644 --- a/src/service/rooms/roles/tests.rs +++ b/src/service/rooms/roles/tests.rs @@ -3,6 +3,8 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use conduwuit_core::matrix::space_roles::RoleDefinition; use ruma::{room_id, OwnedRoomId}; +use super::{compute_user_power_level, roles_satisfy_requirements}; + /// Helper to build a role definitions map. pub fn make_roles(entries: &[(&str, Option)]) -> BTreeMap { entries @@ -31,11 +33,7 @@ pub fn make_requirements(roles: &[&str]) -> HashSet { fn power_level_single_role() { let roles = make_roles(&[("admin", Some(100)), ("mod", Some(50))]); let user_assigned = make_user_roles(&["admin"]); - let max_pl = user_assigned - .iter() - .filter_map(|r| roles.get(r)?.power_level) - .max(); - assert_eq!(max_pl, Some(100)); + assert_eq!(compute_user_power_level(&roles, &user_assigned), Some(100)); } #[test] @@ -46,90 +44,70 @@ fn power_level_multiple_roles_takes_highest() { ("helper", Some(25)), ]); let user_assigned = make_user_roles(&["mod", "helper"]); - let max_pl = user_assigned - .iter() - .filter_map(|r| roles.get(r)?.power_level) - .max(); - assert_eq!(max_pl, Some(50)); + assert_eq!(compute_user_power_level(&roles, &user_assigned), Some(50)); } #[test] fn power_level_no_power_roles() { let roles = make_roles(&[("nsfw", None), ("vip", None)]); let user_assigned = make_user_roles(&["nsfw", "vip"]); - let max_pl = user_assigned - .iter() - .filter_map(|r| roles.get(r)?.power_level) - .max(); - assert_eq!(max_pl, None); + assert_eq!(compute_user_power_level(&roles, &user_assigned), None); } #[test] fn power_level_mixed_roles() { let roles = make_roles(&[("mod", Some(50)), ("nsfw", None)]); let user_assigned = make_user_roles(&["mod", "nsfw"]); - let max_pl = user_assigned - .iter() - .filter_map(|r| roles.get(r)?.power_level) - .max(); - assert_eq!(max_pl, Some(50)); + assert_eq!(compute_user_power_level(&roles, &user_assigned), Some(50)); } #[test] fn power_level_no_roles_assigned() { let roles = make_roles(&[("admin", Some(100))]); let user_assigned: HashSet = HashSet::new(); - let max_pl = user_assigned - .iter() - .filter_map(|r| roles.get(r)?.power_level) - .max(); - assert_eq!(max_pl, None); + assert_eq!(compute_user_power_level(&roles, &user_assigned), None); } #[test] fn power_level_unknown_role_ignored() { let roles = make_roles(&[("admin", Some(100))]); let user_assigned = make_user_roles(&["nonexistent"]); - let max_pl = user_assigned - .iter() - .filter_map(|r| roles.get(r)?.power_level) - .max(); - assert_eq!(max_pl, None); + assert_eq!(compute_user_power_level(&roles, &user_assigned), None); } #[test] fn qualifies_with_all_required_roles() { let required = make_requirements(&["nsfw", "vip"]); let user_assigned = make_user_roles(&["nsfw", "vip", "extra"]); - assert!(required.iter().all(|r| user_assigned.contains(r))); + assert!(roles_satisfy_requirements(&required, &user_assigned)); } #[test] fn does_not_qualify_missing_one_role() { let required = make_requirements(&["nsfw", "vip"]); let user_assigned = make_user_roles(&["nsfw"]); - assert!(!required.iter().all(|r| user_assigned.contains(r))); + assert!(!roles_satisfy_requirements(&required, &user_assigned)); } #[test] fn qualifies_with_no_requirements() { let required: HashSet = HashSet::new(); let user_assigned = make_user_roles(&["nsfw"]); - assert!(required.iter().all(|r| user_assigned.contains(r))); + assert!(roles_satisfy_requirements(&required, &user_assigned)); } #[test] fn does_not_qualify_with_no_roles() { let required = make_requirements(&["nsfw"]); let user_assigned: HashSet = HashSet::new(); - assert!(!required.iter().all(|r| user_assigned.contains(r))); + assert!(!roles_satisfy_requirements(&required, &user_assigned)); } #[test] fn qualifies_empty_requirements_empty_roles() { let required: HashSet = HashSet::new(); let user_assigned: HashSet = HashSet::new(); - assert!(required.iter().all(|r| user_assigned.contains(r))); + assert!(roles_satisfy_requirements(&required, &user_assigned)); } #[test] diff --git a/src/service/rooms/timeline/append.rs b/src/service/rooms/timeline/append.rs index eb6657d9..5f48dcf5 100644 --- a/src/service/rooms/timeline/append.rs +++ b/src/service/rooms/timeline/append.rs @@ -379,7 +379,7 @@ where roles.handle_state_event_change( room_id.to_owned(), event_type_str, - state_key.to_string(), + state_key.to_owned(), ); } }, diff --git a/src/service/rooms/timeline/build.rs b/src/service/rooms/timeline/build.rs index cfe18df6..5f0eebdb 100644 --- a/src/service/rooms/timeline/build.rs +++ b/src/service/rooms/timeline/build.rs @@ -104,6 +104,9 @@ pub async fn build_and_append_pdu( } // Space permission cascading: reject power level changes that conflict // with Space-granted levels (exempt the server user so sync_power_levels works) + type SpaceEnforcementData = + (ruma::OwnedRoomId, Vec<(OwnedUserId, HashSet)>, BTreeMap); + if self.services.roles.is_enabled() && *pdu.kind() == TimelineEventType::RoomPowerLevels && pdu.sender() != >::as_ref(&self.services.globals.server_user) @@ -129,9 +132,6 @@ pub async fn build_and_append_pdu( // Also check that space-managed users aren't omitted // Clone data out of guards to avoid holding locks across await - type SpaceEnforcementData = - (ruma::OwnedRoomId, Vec<(OwnedUserId, HashSet)>, BTreeMap); - let space_data: Vec = { let user_roles_guard = self.services.roles.user_roles.read().await; let roles_guard = self.services.roles.roles.read().await;