Skip to content
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.

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

Accept any string as a key for m.direct account data #4228

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
117 changes: 64 additions & 53 deletions Cargo.lock

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ once_cell = "1.16.0"
pin-project-lite = "0.2.9"
rand = "0.8.5"
reqwest = { version = "0.12.4", default-features = false }
ruma = { version = "0.11.1", features = [
ruma = { git = "https://github.com/ruma/ruma.git", rev = "35fda7f2f7811156df2e60b223dbf136fc143bc8", features = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Potentially stating the obvious here, but if you wanna do a crates.io release soon (I think this was planned), make sure to only merge this PR afterwards 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

New policy in town, no git deps anymore. That's why cargo-deny makes the CI fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh. Well I don't think we'll have the Ruma change blocking this PR out on crates.io anytime soon.

Copy link
Contributor Author

@MatMaul MatMaul Nov 14, 2024

Choose a reason for hiding this comment

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

No rush on my side, it can wait.

Feel free to review in the meantime, so we can merge when we got a ruma release out.

"client-api-c",
"compat-upload-signatures",
"compat-user-id",
Expand All @@ -59,7 +59,7 @@ ruma = { version = "0.11.1", features = [
"unstable-msc4075",
"unstable-msc4140",
] }
ruma-common = "0.14.1"
ruma-common = { git = "https://github.com/ruma/ruma.git", rev = "35fda7f2f7811156df2e60b223dbf136fc143bc8" }
serde = "1.0.151"
serde_html_form = "0.2.0"
serde_json = "1.0.91"
Expand Down
12 changes: 7 additions & 5 deletions crates/matrix-sdk-base/src/response_processors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@ use std::{
};

use ruma::{
events::{AnyGlobalAccountDataEvent, GlobalAccountDataEventType},
events::{
direct::OwnedDirectUserIdentifier, AnyGlobalAccountDataEvent, GlobalAccountDataEventType,
},
serde::Raw,
OwnedUserId, RoomId,
RoomId,
};
use tracing::{debug, instrument, trace, warn};

Expand Down Expand Up @@ -94,10 +96,10 @@ impl AccountDataProcessor {
for event in events {
let AnyGlobalAccountDataEvent::Direct(direct_event) = event else { continue };

let mut new_dms = HashMap::<&RoomId, HashSet<OwnedUserId>>::new();
for (user_id, rooms) in direct_event.content.iter() {
let mut new_dms = HashMap::<&RoomId, HashSet<OwnedDirectUserIdentifier>>::new();
for (user_identifier, rooms) in direct_event.content.iter() {
for room_id in rooms {
new_dms.entry(room_id).or_default().insert(user_id.clone());
new_dms.entry(room_id).or_default().insert(user_identifier.clone());
}
}

Expand Down
3 changes: 2 additions & 1 deletion crates/matrix-sdk-base/src/rooms/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use ruma::{
events::{
beacon_info::BeaconInfoEventContent,
call::member::{CallMemberEventContent, CallMemberStateKey},
direct::OwnedDirectUserIdentifier,
macros::EventContent,
room::{
avatar::RoomAvatarEventContent,
Expand Down Expand Up @@ -127,7 +128,7 @@ pub struct BaseRoomInfo {
pub(crate) create: Option<MinimalStateEvent<RoomCreateWithCreatorEventContent>>,
/// A list of user ids this room is considered as direct message, if this
/// room is a DM.
pub(crate) dm_targets: HashSet<OwnedUserId>,
pub(crate) dm_targets: HashSet<OwnedDirectUserIdentifier>,
/// The `m.room.encryption` event content that enabled E2EE in this room.
pub(crate) encryption: Option<RoomEncryptionEventContent>,
/// The guest access policy of this room.
Expand Down
3 changes: 2 additions & 1 deletion crates/matrix-sdk-base/src/rooms/normal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ use ruma::{
api::client::sync::sync_events::v3::RoomSummary as RumaSummary,
events::{
call::member::{CallMemberStateKey, MembershipData},
direct::OwnedDirectUserIdentifier,
ignored_user_list::IgnoredUserListEventContent,
receipt::{Receipt, ReceiptThread, ReceiptType},
room::{
Expand Down Expand Up @@ -460,7 +461,7 @@ impl Room {
/// only be considered as guidance. We leave members in this list to allow
/// us to re-find a DM with a user even if they have left, since we may
/// want to re-invite them.
pub fn direct_targets(&self) -> HashSet<OwnedUserId> {
pub fn direct_targets(&self) -> HashSet<OwnedDirectUserIdentifier> {
self.inner.read().base_info.dm_targets.clone()
}

Expand Down
28 changes: 15 additions & 13 deletions crates/matrix-sdk-base/src/sliding_sync/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -905,7 +905,7 @@ mod tests {
api::client::sync::sync_events::UnreadNotificationsCount,
assign, event_id,
events::{
direct::DirectEventContent,
direct::{DirectEventContent, DirectUserIdentifier, OwnedDirectUserIdentifier},
room::{
avatar::RoomAvatarEventContent,
canonical_alias::RoomCanonicalAliasEventContent,
Expand Down Expand Up @@ -1331,7 +1331,7 @@ mod tests {
create_dm(&client, room_id, user_a_id, user_b_id, MembershipState::Join).await;

// (Sanity: B is a direct target, and is in Join state)
assert!(direct_targets(&client, room_id).contains(user_b_id));
assert!(direct_targets(&client, room_id).contains(<&DirectUserIdentifier>::from(user_b_id)));
assert_eq!(membership(&client, room_id, user_b_id).await, MembershipState::Join);

// When B leaves
Expand All @@ -1340,7 +1340,7 @@ mod tests {
// Then B is still a direct target, and is in Leave state (B is a direct target
// because we want to return to our old DM in the UI even if the other
// user left, so we can reinvite them. See https://github.com/matrix-org/matrix-rust-sdk/issues/2017)
assert!(direct_targets(&client, room_id).contains(user_b_id));
assert!(direct_targets(&client, room_id).contains(<&DirectUserIdentifier>::from(user_b_id)));
assert_eq!(membership(&client, room_id, user_b_id).await, MembershipState::Leave);
}

Expand All @@ -1356,7 +1356,7 @@ mod tests {
create_dm(&client, room_id, user_a_id, user_b_id, MembershipState::Invite).await;

// (Sanity: B is a direct target, and is in Invite state)
assert!(direct_targets(&client, room_id).contains(user_b_id));
assert!(direct_targets(&client, room_id).contains(<&DirectUserIdentifier>::from(user_b_id)));
assert_eq!(membership(&client, room_id, user_b_id).await, MembershipState::Invite);

// When B declines the invitation (i.e. leaves)
Expand All @@ -1365,7 +1365,7 @@ mod tests {
// Then B is still a direct target, and is in Leave state (B is a direct target
// because we want to return to our old DM in the UI even if the other
// user left, so we can reinvite them. See https://github.com/matrix-org/matrix-rust-sdk/issues/2017)
assert!(direct_targets(&client, room_id).contains(user_b_id));
assert!(direct_targets(&client, room_id).contains(<&DirectUserIdentifier>::from(user_b_id)));
assert_eq!(membership(&client, room_id, user_b_id).await, MembershipState::Leave);
}

Expand All @@ -1383,7 +1383,7 @@ mod tests {
assert_eq!(membership(&client, room_id, user_a_id).await, MembershipState::Join);

// (Sanity: B is a direct target, and is in Join state)
assert!(direct_targets(&client, room_id).contains(user_b_id));
assert!(direct_targets(&client, room_id).contains(<&DirectUserIdentifier>::from(user_b_id)));
assert_eq!(membership(&client, room_id, user_b_id).await, MembershipState::Join);

let room = client.get_room(room_id).unwrap();
Expand All @@ -1407,7 +1407,7 @@ mod tests {
assert_eq!(membership(&client, room_id, user_a_id).await, MembershipState::Join);

// (Sanity: B is a direct target, and is in Join state)
assert!(direct_targets(&client, room_id).contains(user_b_id));
assert!(direct_targets(&client, room_id).contains(<&DirectUserIdentifier>::from(user_b_id)));
assert_eq!(membership(&client, room_id, user_b_id).await, MembershipState::Invite);

let room = client.get_room(room_id).unwrap();
Expand Down Expand Up @@ -2552,9 +2552,10 @@ mod tests {
let mut room_response = http::response::Room::new();
set_room_joined(&mut room_response, user_a_id);
let mut response = response_with_room(room_id_1, room_response);
let mut direct_content = BTreeMap::new();
direct_content.insert(user_a_id.to_owned(), vec![room_id_1.to_owned()]);
direct_content.insert(user_b_id.to_owned(), vec![room_id_2.to_owned()]);
let mut direct_content: BTreeMap<OwnedDirectUserIdentifier, Vec<OwnedRoomId>> =
BTreeMap::new();
direct_content.insert(user_a_id.into(), vec![room_id_1.to_owned()]);
direct_content.insert(user_b_id.into(), vec![room_id_2.to_owned()]);
response
.extensions
.account_data
Expand Down Expand Up @@ -2665,7 +2666,7 @@ mod tests {
member.membership().clone()
}

fn direct_targets(client: &BaseClient, room_id: &RoomId) -> HashSet<OwnedUserId> {
fn direct_targets(client: &BaseClient, room_id: &RoomId) -> HashSet<OwnedDirectUserIdentifier> {
let room = client.get_room(room_id).expect("Room not found!");
room.direct_targets()
}
Expand Down Expand Up @@ -2724,8 +2725,9 @@ mod tests {
user_id: OwnedUserId,
room_ids: Vec<OwnedRoomId>,
) {
let mut direct_content = BTreeMap::new();
direct_content.insert(user_id, room_ids);
let mut direct_content: BTreeMap<OwnedDirectUserIdentifier, Vec<OwnedRoomId>> =
BTreeMap::new();
direct_content.insert(user_id.into(), room_ids);
response
.extensions
.account_data
Expand Down
5 changes: 3 additions & 2 deletions crates/matrix-sdk-base/src/store/migration_helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use std::{
use matrix_sdk_common::deserialized_responses::SyncTimelineEvent;
use ruma::{
events::{
direct::OwnedDirectUserIdentifier,
room::{
avatar::RoomAvatarEventContent,
canonical_alias::RoomCanonicalAliasEventContent,
Expand All @@ -37,7 +38,7 @@ use ruma::{
},
EmptyStateKey, EventContent, RedactContent, StateEventContent, StateEventType,
},
OwnedRoomId, OwnedUserId, RoomId,
OwnedRoomId, RoomId,
};
use serde::{Deserialize, Serialize};

Expand Down Expand Up @@ -155,7 +156,7 @@ fn encryption_state_default() -> bool {
struct BaseRoomInfoV1 {
avatar: Option<MinimalStateEvent<RoomAvatarEventContent>>,
canonical_alias: Option<MinimalStateEvent<RoomCanonicalAliasEventContent>>,
dm_targets: HashSet<OwnedUserId>,
dm_targets: HashSet<OwnedDirectUserIdentifier>,
encryption: Option<RoomEncryptionEventContent>,
guest_access: Option<MinimalStateEvent<RoomGuestAccessEventContent>>,
history_visibility: Option<MinimalStateEvent<RoomHistoryVisibilityEventContent>>,
Expand Down
2 changes: 1 addition & 1 deletion crates/matrix-sdk/src/account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -861,7 +861,7 @@ impl Account {
};

for user_id in user_ids {
content.entry(user_id.to_owned()).or_default().push(room_id.to_owned());
content.entry(user_id.into()).or_default().push(room_id.to_owned());
}

// TODO: We should probably save the fact that we need to send this out
Expand Down
7 changes: 5 additions & 2 deletions crates/matrix-sdk/src/encryption/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ use ruma::{
uiaa::{AuthData, UiaaInfo},
},
assign,
events::room::{MediaSource, ThumbnailInfo},
events::{
direct::DirectUserIdentifier,
room::{MediaSource, ThumbnailInfo},
},
DeviceId, OwnedDeviceId, OwnedUserId, TransactionId, UserId,
};
use serde::Deserialize;
Expand Down Expand Up @@ -607,7 +610,7 @@ impl Client {
// Find the room we share with the `user_id` and only with `user_id`
let room = rooms.into_iter().find(|r| {
let targets = r.direct_targets();
targets.len() == 1 && targets.contains(user_id)
targets.len() == 1 && targets.contains(<&DirectUserIdentifier>::from(user_id))
});

trace!(?room, "Found room");
Expand Down
2 changes: 1 addition & 1 deletion crates/matrix-sdk/src/room/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1205,7 +1205,7 @@ impl Room {
room_members.retain(|member| member.user_id() != self.own_user_id());

for member in room_members {
let entry = content.entry(member.user_id().to_owned()).or_default();
let entry = content.entry(member.user_id().into()).or_default();
if !entry.iter().any(|room_id| room_id == this_room_id) {
entry.push(this_room_id.to_owned());
}
Expand Down
9 changes: 7 additions & 2 deletions crates/matrix-sdk/tests/integration/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@ use ruma::{
assign, device_id,
directory::Filter,
event_id,
events::{direct::DirectEventContent, AnyInitialStateEvent},
events::{
direct::{DirectEventContent, OwnedDirectUserIdentifier},
AnyInitialStateEvent,
},
room_id,
serde::Raw,
user_id, OwnedUserId,
Expand Down Expand Up @@ -496,7 +499,9 @@ async fn test_marking_room_as_dm() {
"The body of the PUT /account_data request should be a valid DirectEventContent",
);

let bob_entry = content.get(bob).expect("We should have bob in the direct event content");
let bob_entry = content
.get(&OwnedDirectUserIdentifier::from(bob.to_owned()))
.expect("We should have bob in the direct event content");

assert_eq!(content.len(), 2, "We should have entries for bob and foo");
assert_eq!(bob_entry.len(), 3, "Bob should have 3 direct rooms");
Expand Down
3 changes: 2 additions & 1 deletion crates/matrix-sdk/tests/integration/room/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use matrix_sdk_test::{
use ruma::{
event_id,
events::{
direct::DirectUserIdentifier,
room::{
avatar::{self, RoomAvatarEventContent},
member::MembershipState,
Expand Down Expand Up @@ -833,7 +834,7 @@ async fn test_is_direct() {
// The room is direct now.
let direct_targets = room.direct_targets();
assert_eq!(direct_targets.len(), 1);
assert!(direct_targets.contains(*BOB));
assert!(direct_targets.contains(<&DirectUserIdentifier>::from(*BOB)));
assert!(room.is_direct().await.unwrap());

// Unset the room as direct.
Expand Down
35 changes: 34 additions & 1 deletion crates/matrix-sdk/tests/integration/room/joined.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,14 @@ use ruma::{
api::client::{membership::Invite3pidInit, receipt::create_receipt::v3::ReceiptType},
assign, event_id,
events::{
direct::DirectUserIdentifier,
receipt::ReceiptThread,
room::message::{RoomMessageEventContent, RoomMessageEventContentWithoutRelation},
TimelineEventType,
},
int, mxc_uri, owned_event_id, room_id, thirdparty, user_id, OwnedUserId, TransactionId,
};
use serde_json::{json, Value};
use serde_json::{from_value, json, Value};
use wiremock::{
matchers::{body_json, body_partial_json, header, method, path_regex},
Mock, ResponseTemplate,
Expand Down Expand Up @@ -630,6 +631,38 @@ async fn test_reset_power_levels() {
room.reset_power_levels().await.unwrap();
}

#[async_test]
async fn test_is_direct_invite_by_3pid() {
let (client, server) = logged_in_client_with_server().await;

let mut sync_builder = SyncResponseBuilder::new();
sync_builder.add_joined_room(JoinedRoomBuilder::default());
let data = json!({
"content": {
"[email protected]": [*DEFAULT_TEST_ROOM_ID],
},
"event_id": "$757957878228ekrDs:localhost",
"origin_server_ts": 17195787,
"sender": "@example:localhost",
"state_key": "",
"type": "m.direct",
"unsigned": {
"age": 139298
}
});
sync_builder.add_global_account_data_bulk(vec![from_value(data).unwrap()]);

mock_sync(&server, sync_builder.build_json_sync_response(), None).await;
mock_encryption_state(&server, false).await;

let sync_settings = SyncSettings::new().timeout(Duration::from_millis(3000));
let _response = client.sync_once(sync_settings).await.unwrap();

let room = client.get_room(&DEFAULT_TEST_ROOM_ID).unwrap();
assert!(room.is_direct().await.unwrap());
assert!(room.direct_targets().contains(<&DirectUserIdentifier>::from("[email protected]")));
}

#[async_test]
async fn test_call_notifications_ring_for_dms() {
let (client, server) = logged_in_client_with_server().await;
Expand Down
12 changes: 9 additions & 3 deletions crates/matrix-sdk/tests/integration/room/left.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ use matrix_sdk_test::{
async_test, test_json, GlobalAccountDataTestEvent, LeftRoomBuilder, SyncResponseBuilder,
DEFAULT_TEST_ROOM_ID,
};
use ruma::{events::direct::DirectEventContent, user_id, OwnedRoomOrAliasId};
use ruma::{
events::direct::{DirectEventContent, DirectUserIdentifier, OwnedDirectUserIdentifier},
user_id, OwnedRoomOrAliasId,
};
use serde_json::json;
use wiremock::{
matchers::{header, method, path, path_regex},
Expand Down Expand Up @@ -70,7 +73,7 @@ async fn test_forget_direct_room() {
let room = client.get_room(&DEFAULT_TEST_ROOM_ID).unwrap();
assert_eq!(room.state(), RoomState::Left);
assert!(room.is_direct().await.unwrap());
assert!(room.direct_targets().contains(invited_user_id));
assert!(room.direct_targets().contains(<&DirectUserIdentifier>::from(invited_user_id)));

let direct_account_data = client
.account()
Expand All @@ -80,7 +83,10 @@ async fn test_forget_direct_room() {
.expect("no m.direct account data")
.deserialize()
.expect("failed to deserialize m.direct account data");
assert_matches!(direct_account_data.get(invited_user_id), Some(invited_user_dms));
assert_matches!(
direct_account_data.get(&OwnedDirectUserIdentifier::from(invited_user_id)),
Some(invited_user_dms)
);
assert_eq!(invited_user_dms, &[DEFAULT_TEST_ROOM_ID.to_owned()]);

Mock::given(method("POST"))
Expand Down
Loading