ember/continuwuity
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

fix(spaces): address critical review findings
Some checks failed
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Details
Checks / Prek / Pre-commit & Formatting (pull_request) Failing after 6s
Details
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 6s
Details
Update flake hashes / update-flake-hashes (pull_request) Failing after 5s
Details

Browse source 
- Fix pending_enforcement cleanup: wrap enforcement body in async block
  so removal always runs even on semaphore error or early return
- Fix check_join_allowed: remove is_enabled() guard that blocked
  per-space overrides when global flag is false (get_parent_spaces
  already filters by is_enabled_for_space)
- Fix kick/join asymmetry: kick_unqualified_from_rooms now checks all
  parent spaces before kicking, matching check_join_allowed's OR logic
- Fix lock ordering: validate_pl_change now acquires roles before
  user_roles, matching get_user_power_level's order
- Fix ensure_default_roles TOCTOU: move existence check inside state
  lock to prevent concurrent duplicate writes
This commit is contained in:
ember33 2026-03-19 18:41:54 +01:00
parent 5740f72b4d
commit 89074c9741
1 changed files with 66 additions and 56 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

122
src/service/rooms/roles/mod.rs
View file

@ -205,6 +205,9 @@ pub async fn ensure_default_roles(&self, space_id: &RoomId) -> Result {
return Ok(());
}
let sender = self.services.globals.server_user.as_ref();
let state_lock = self.services.state.mutex.lock(space_id).await;
let roles_event_type = roles_event_type();
if self
.services
@ -228,9 +231,6 @@ pub async fn ensure_default_roles(&self, space_id: &RoomId) -> Result {
let content = SpaceRolesEventContent { roles };
let sender = self.services.globals.server_user.as_ref();
let state_lock = self.services.state.mutex.lock(space_id).await;
let pdu = PduBuilder {
event_type: ruma::events::TimelineEventType::from(SPACE_ROLES_EVENT_TYPE.to_owned()),
content: to_raw_value(&content).map_err(|e| {
@ -733,8 +733,8 @@ pub async fn validate_pl_change(
}
let space_data: Vec<(Vec<(OwnedUserId, HashSet<String>)>, BTreeMap<String, RoleDefinition>)> = {
let user_roles_guard = self.user_roles.read().await;
let roles_guard = self.roles.read().await;
let user_roles_guard = self.user_roles.read().await;
parent_spaces
.iter()
.filter_map(|ps| {
@ -798,10 +798,6 @@ pub async fn validate_pl_change(
/// Space roles to join a room.
#[implement(Service)]
pub async fn check_join_allowed(&self, room_id: &RoomId, user_id: &UserId) -> Result {
if !self.is_enabled() {
return Ok(());
}
let parent_spaces = self.get_parent_spaces(room_id).await;
if parent_spaces.is_empty() {
return Ok(());
@ -851,65 +847,71 @@ impl Service {
pending.insert(space_id.clone())
};
let Ok(_permit) = this.enforcement_semaphore.acquire().await else {
return;
};
async {
let Ok(_permit) = this.enforcement_semaphore.acquire().await else {
return;
};
this.populate_space(&space_id).await;
this.populate_space(&space_id).await;
match event_type.as_str() {
| SPACE_ROLES_EVENT_TYPE => {
this.sync_power_levels_for_children(&space_id).await;
let space_members: Vec<OwnedUserId> = this
.services
.state_cache
.room_members(&space_id)
.map(ToOwned::to_owned)
.collect()
.await;
for member in &space_members {
if let Err(e) =
Box::pin(this.kick_unqualified_from_rooms(&space_id, member)).await
{
debug_warn!(user_id = %member, error = ?e, "Role definition revalidation kick failed");
}
}
},
| SPACE_ROLE_MEMBER_EVENT_TYPE => {
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!(user_id = %user_id, error = ?e, "Space role auto-join failed");
}
if let Err(e) =
Box::pin(this.kick_unqualified_from_rooms(&space_id, user_id)).await
{
debug_warn!(user_id = %user_id, error = ?e, "Space role auto-kick failed");
}
match event_type.as_str() {
| SPACE_ROLES_EVENT_TYPE => {
this.sync_power_levels_for_children(&space_id).await;
}
},
| SPACE_ROLE_ROOM_EVENT_TYPE => {
if let Ok(target_room) = RoomId::parse(state_key.as_str()) {
let members: Vec<OwnedUserId> = this
let space_members: Vec<OwnedUserId> = this
.services
.state_cache
.room_members(target_room)
.room_members(&space_id)
.map(ToOwned::to_owned)
.collect()
.await;
for member in &members {
for member in &space_members {
if let Err(e) =
Box::pin(this.kick_unqualified_from_rooms(&space_id, member))
.await
{
debug_warn!(user_id = %member, error = ?e, "Space role requirement kick failed");
debug_warn!(user_id = %member, error = ?e, "Role definition revalidation kick failed");
}
}
}
},
| _ => {},
},
| SPACE_ROLE_MEMBER_EVENT_TYPE => {
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!(user_id = %user_id, error = ?e, "Space role auto-join failed");
}
if let Err(e) =
Box::pin(this.kick_unqualified_from_rooms(&space_id, user_id))
.await
{
debug_warn!(user_id = %user_id, error = ?e, "Space role auto-kick failed");
}
this.sync_power_levels_for_children(&space_id).await;
}
},
| SPACE_ROLE_ROOM_EVENT_TYPE => {
if let Ok(target_room) = RoomId::parse(state_key.as_str()) {
let members: Vec<OwnedUserId> = this
.services
.state_cache
.room_members(target_room)
.map(ToOwned::to_owned)
.collect()
.await;
for member in &members {
if let Err(e) =
Box::pin(this.kick_unqualified_from_rooms(&space_id, member))
.await
{
debug_warn!(user_id = %member, error = ?e, "Space role requirement kick failed");
}
}
}
},
| _ => {},
}
}
.await;
this.pending_enforcement.write().await.remove(&space_id);
});
@ -1077,10 +1079,18 @@ pub async fn kick_unqualified_from_rooms(&self, space_id: &RoomId, user_id: &Use
continue;
}
if self
.user_qualifies_for_room(space_id, child_room_id, user_id)
.await
{
let all_parents = self.get_parent_spaces(child_room_id).await;
let mut qualifies_in_any = false;
for parent in &all_parents {
if self
.user_qualifies_for_room(parent, child_room_id, user_id)
.await
{
qualifies_in_any = true;
break;
}
}
if qualifies_in_any {
continue;
}