Skip to content

Verify membership when test if name is ambiguous #5780

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
multisme wants to merge 9 commits into
base: main
Choose a base branch
from
+238 −6
Draft

Verify membership when test if name is ambiguous #5780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b5316ef
Verify membership when test if name is ambiguous
multisme Oct 13, 2025
932cea5
Following the spec more precisely
multisme Oct 14, 2025
79beb6c
Separate both conditions for readability
multisme Oct 14, 2025
7cd077d
Non working tests
multisme Oct 14, 2025
48f8549
Setting the map_ambiguity's changes key in the tests
multisme Oct 15, 2025
cd9e7e8
Solved the membership check by creating an active_users variable in M…
multisme Oct 16, 2025
2892f60
Formating imports
multisme Oct 16, 2025
4ca8502
Add new get_active_users_with_display_name trait
multisme Jan 6, 2026
b84b451
Add new get_active_users_with_display_name functions to sqlite store
multisme Jan 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 119 additions & 5 deletions crates/matrix-sdk-base/src/room/members.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,10 +179,13 @@ impl Room {
.transpose()?
.map(|e| e.content.ignored_users.into_keys().collect());

let active_users = self.store.get_user_ids(self.room_id(), RoomMemberships::ACTIVE).await?;

Ok(MemberRoomInfo {
power_levels: power_levels.into(),
max_power_level,
users_display_names,
active_users,
ignored_users,
})
}
Expand Down Expand Up @@ -211,13 +214,24 @@ impl RoomMember {
presence: Option<PresenceEvent>,
room_info: &MemberRoomInfo<'_>,
) -> Self {
let MemberRoomInfo { power_levels, max_power_level, users_display_names, ignored_users } =
room_info;
let MemberRoomInfo {
power_levels,
max_power_level,
users_display_names,
ignored_users,
active_users,
} = room_info;

let display_name = event.display_name();
let display_name_ambiguous = users_display_names
.get(&display_name)
.is_some_and(|s| is_display_name_ambiguous(&display_name, s));

let display_name_ambiguous = users_display_names.get(&display_name).is_some_and(|s| {
// s.filter(|n| )
Copy link
Member

@Hywan Hywan Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be removed

Copy link
Contributor Author

@multisme multisme Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, taking care of that now.

Copy link
Contributor Author

@multisme multisme Jan 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I looked back into it, and the reason I didn't remove it was that I would need to make is_display_name_ambiguous accept the list of current active users, which imply a lot of changes in the matrix-sdk-base/src/store/ambiguity_map.rs, so I wanted to confirm before starting that work.

Copy link
Member

@Hywan Hywan Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can as new commits and see where it goes. If it's too complex, we will take another path. Thanks for asking.

if !is_display_name_ambiguous(&display_name, s) {
return false;
}
//We check of many active_users with the same surname exist
active_users.iter().filter(|u| s.contains(*u)).count() > 1
});
let is_ignored = ignored_users.as_ref().is_some_and(|s| s.contains(event.user_id()));

Self {
Expand Down Expand Up @@ -390,6 +404,7 @@ pub(crate) struct MemberRoomInfo<'a> {
pub(crate) max_power_level: i64,
pub(crate) users_display_names: HashMap<&'a DisplayName, BTreeSet<OwnedUserId>>,
pub(crate) ignored_users: Option<BTreeSet<OwnedUserId>>,
pub(crate) active_users: Vec<OwnedUserId>,
}

/// The kind of room member updates that just happened.
Expand Down Expand Up @@ -478,9 +493,23 @@ pub fn normalize_power_level(power_level: Int, max_power_level: i64) -> Int {

#[cfg(test)]
mod tests {
use std::sync::Arc;

use matrix_sdk_test::{async_test, event_factory::EventFactory};
use proptest::prelude::*;
use ruma::{room_id, user_id};

use super::*;
use crate::{RoomState, StateChanges, StateStore, store::MemoryStore};

fn make_room_test_helper(room_type: RoomState) -> (Arc<MemoryStore>, Room) {
let store = Arc::new(MemoryStore::new());
let user_id = user_id!("@me:example.org");
let room_id = room_id!("!test:localhost");
let (sender, _receiver) = tokio::sync::broadcast::channel(1);

(store.clone(), Room::new(user_id, store, room_id, room_type, sender))
}

prop_compose! {
fn arb_int()(id in any::<i64>()) -> Int {
Expand Down Expand Up @@ -526,4 +555,89 @@ mod tests {
assert!(normalized >= 0);
assert!(normalized <= 100);
}

#[async_test]
async fn test_room_member_from_parts() {
let (store, room) = make_room_test_helper(RoomState::Joined);

let carol = user_id!("@carol:example.org");
let denis = user_id!("@denis:example.org");
let erica = user_id!("@erica:example.org");
let fred = user_id!("@fred:example.org");
let fredo = user_id!("@fredo:example.org");
let bob = user_id!("@bob:example.org");
let julie = user_id!("@julie:example.org");
let me = user_id!("@me:example.org");
let mewto = user_id!("@mewto:example.org");

let mut changes = StateChanges::new("".to_owned());

let f = EventFactory::new().room(room_id!("!test:localhost"));

{
let members = changes
.state
.entry(room.room_id().to_owned())
.or_default()
.entry(StateEventType::RoomMember)
.or_default();

let ambiguity_maps =
changes.ambiguity_maps.entry(room.room_id().to_owned()).or_default();

let display_name = DisplayName::new("Carol");
members.insert(carol.into(), f.member(carol).display_name("Carol").into());
ambiguity_maps.entry(display_name).or_default().insert(carol.to_owned());

let display_name = DisplayName::new("Fred");
members.insert(fred.into(), f.member(fred).display_name("Fred").into());
ambiguity_maps.entry(display_name.clone()).or_default().insert(fred.to_owned());
members.insert(
fredo.into(),
f.member(fredo).display_name("Fred").membership(MembershipState::Knock).into(),
);
ambiguity_maps.entry(display_name.clone()).or_default().insert(fredo.to_owned());
members.insert(
denis.into(),
f.member(denis).display_name("Fred").membership(MembershipState::Leave).into(),
);
ambiguity_maps.entry(display_name.clone()).or_default().insert(erica.to_owned());
members.insert(
erica.into(),
f.member(erica).display_name("Fred").membership(MembershipState::Ban).into(),
);

let display_name = DisplayName::new("Bob");
members.insert(
bob.into(),
f.member(bob).display_name("Bob").membership(MembershipState::Invite).into(),
);
ambiguity_maps.entry(display_name.clone()).or_default().insert(bob.to_owned());
members.insert(julie.into(), f.member(me).display_name("Bob").into());
ambiguity_maps.entry(display_name.clone()).or_default().insert(julie.to_owned());

let display_name = DisplayName::new("Me");
members.insert(me.into(), f.member(me).display_name("Me").into());
ambiguity_maps.entry(display_name.clone()).or_default().insert(me.to_owned());
members.insert(mewto.into(), f.member(mewto).display_name("Me").into());
ambiguity_maps.entry(display_name.clone()).or_default().insert(mewto.to_owned());

store.save_changes(&changes).await.unwrap();
}

assert!(!room.get_member(carol).await.unwrap().expect("Carol user").name_ambiguous());

assert!(!room.get_member(fred).await.unwrap().expect("Fred user").name_ambiguous());
assert!(!room.get_member(fredo).await.unwrap().expect("Fredo user").name_ambiguous());
assert!(!room.get_member(denis).await.unwrap().expect("Denis user").name_ambiguous());
assert!(!room.get_member(erica).await.unwrap().expect("Erica user").name_ambiguous());

assert!(!room.get_member(bob).await.unwrap().expect("Bob user").name_ambiguous());

assert!(!room.get_member(julie).await.unwrap().expect("Julie user").name_ambiguous());
assert!(!room.get_member(bob).await.unwrap().expect("Bob user").name_ambiguous());

assert!(room.get_member(me).await.unwrap().expect("Me user").name_ambiguous());
assert!(room.get_member(mewto).await.unwrap().expect("Mewto user").name_ambiguous());
}
}
24 changes: 24 additions & 0 deletions crates/matrix-sdk-base/src/store/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,30 @@ pub trait StateStore: AsyncTraitDeps {
display_names: &'a [DisplayName],
) -> Result<HashMap<&'a DisplayName, BTreeSet<OwnedUserId>>, Self::Error>;

/// Get an event out of the account data store.
///
/// # Arguments
///
/// * `event_type` - The event type of the account data event.
async fn get_active_users_with_display_name(
&self,
room_id: &RoomId,
display_name: &DisplayName,
) -> Result<BTreeSet<OwnedUserId>, Self::Error>;

/// Get all the active users that use the given display names in the given room.
///
/// # Arguments
///
/// * `room_id` - The ID of the room to fetch the display names for.
///
/// * `display_names` - The display names that the users use.
async fn get_active_users_with_display_names<'a>(
&self,
room_id: &RoomId,
display_names: &'a [DisplayName],
) -> Result<HashMap<&'a DisplayName, BTreeSet<OwnedUserId>>, Self::Error>;

/// Get an event out of the account data store.
///
/// # Arguments
Expand Down
96 changes: 95 additions & 1 deletion crates/matrix-sdk-sqlite/src/state_store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -996,6 +996,30 @@ trait SqliteObjectStateStoreExt: SqliteAsyncConnExt {
})
.await
}

async fn get_active_display_names(
&self,
room_id: Key,
names: Vec<Key>,
) -> Result<Vec<(Vec<u8>, Vec<u8>)>> {
let names_length = names.len();

self.chunk_large_query_over(names, Some(names_length), move |txn, names| {
let sql_params = repeat_vars(names.len());
let sql = format!(
"SELECT name, data FROM display_name WHERE room_id = ? AND name IN ({sql_params}) AND membership = ?"
);

let params = rusqlite::params_from_iter(iter::once(room_id.clone()).chain(names).chain(iter::once(RoomMemberships::ACTIVE)));

Ok(txn
.prepare(&sql)?
.query(params)?
.mapped(|row| Ok((row.get(0)?, row.get(1)?)))
.collect::<Result<_, _>>()?)
})
.await
}

async fn get_user_receipt(
&self,
Expand Down Expand Up @@ -1652,7 +1676,7 @@ impl StateStore for SqliteStateStore {
.transpose()?
.unwrap_or_default())
}

async fn get_users_with_display_names<'a>(
&self,
room_id: &RoomId,
Expand Down Expand Up @@ -1700,6 +1724,76 @@ impl StateStore for SqliteStateStore {
Ok(result)
}

async fn get_active_users_with_display_name(
&self,
room_id: &RoomId,
display_name: &DisplayName,
) -> Result<BTreeSet<OwnedUserId>> {
let room_id = self.encode_key(keys::DISPLAY_NAME, room_id);
let names = vec![self.encode_key(
keys::DISPLAY_NAME,
display_name.as_normalized_str().unwrap_or_else(|| display_name.as_raw_str()),
)];

Ok(self
.read()
.await?
.get_active_display_names(room_id, names)
.await?
.into_iter()
.next()
.map(|(_, data)| self.deserialize_json(&data))
.transpose()?
.unwrap_or_default())
}

async fn get_active_users_with_display_names<'a>(
&self,
room_id: &RoomId,
display_names: &'a [DisplayName],
) -> Result<HashMap<&'a DisplayName, BTreeSet<OwnedUserId>>> {
let mut result = HashMap::new();

if display_names.is_empty() {
return Ok(result);
}

let room_id = self.encode_key(keys::DISPLAY_NAME, room_id);
let mut names_map = display_names
.iter()
.flat_map(|display_name| {
// We encode the display name as the `raw_str()` and the normalized string.
//
// This is for compatibility reasons since:
// 1. Previously "Alice" and "alice" were considered to be distinct display
// names, while we now consider them to be the same so we need to merge the
// previously distinct buckets of user IDs.
// 2. We can't do a migration to merge the previously distinct buckets of user
// IDs since the display names itself are hashed before they are persisted
// in the store.
let raw =
(self.encode_key(keys::DISPLAY_NAME, display_name.as_raw_str()), display_name);
let normalized = display_name.as_normalized_str().map(|normalized| {
(self.encode_key(keys::DISPLAY_NAME, normalized), display_name)
});

iter::once(raw).chain(normalized)
})
.collect::<BTreeMap<_, _>>();
let names = names_map.keys().cloned().collect();

for (name, data) in self.read().await?.get_active_display_names(room_id, names).await?.into_iter()
{
let display_name =
names_map.remove(name.as_slice()).expect("returned display names were requested");
let user_ids: BTreeSet<_> = self.deserialize_json(&data)?;

result.entry(display_name).or_insert_with(BTreeSet::new).extend(user_ids);
}

Ok(result)
}

async fn get_account_data_event(
&self,
event_type: GlobalAccountDataEventType,
Expand Down
Loading