Skip to content
Merged
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
102 changes: 102 additions & 0 deletions crates/matrix-sdk-base/src/latest_event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,35 @@ pub enum LatestEventValue {
LocalCannotBeSent(LocalLatestEventValue),
}

impl LatestEventValue {
/// Get the timestamp of the [`LatestEventValue`].
///
/// If it's [`None`], it returns `None`. If it's [`Remote`], it returns the
/// `origin_server_ts`. If it's [`LocalIsSending`] or [`LocalCannotBeSent`],
Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed this in chat but the discussion died down, so I'll repeat here.

To avoid malicious homeservers and users manipulating our room ordering using the origin_server_ts can't we record a local timestamp whenever we create a new LatestEventValue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I've been thinking about that.

When we receive an event via the sync, we can record the now() as the local timestamp. However, it means that all events from all rooms for a particular sync will have the same local timestamp, and thus, we can't use that to sort the rooms (because all “recorded timestamps” will be equal).

I think we have to keep origin_server_ts for the moment. We can warn it's a known limitations. It's also used to display the event's time in the timeline for example. If we can't trust this value, it's a protocol issue I suppose. I don't see how to solve that for the moment.

/// it returns the [`timestamp`] value.
///
/// [`None`]: LatestEventValue::None
/// [`Remote`]: LatestEventValue::Remote
/// [`LocalIsSending`]: LatestEventValue::LocalIsSending
/// [`LocalCannotBeSent`]: LatestEventValue::LocalCannotBeSent
/// [`timestamp`]: LocalLatestEventValue::timestamp
pub fn timestamp(&self) -> Option<u64> {
match self {
Self::None => None,
Self::Remote(remote_latest_event_value) => remote_latest_event_value
.kind
.raw()
.get_field::<u64>("origin_server_ts")
.ok()
.flatten(),
Self::LocalIsSending(LocalLatestEventValue { timestamp, .. })
| Self::LocalCannotBeSent(LocalLatestEventValue { timestamp, .. }) => {
Some(timestamp.get().into())
}
}
}
}

/// Represents the value for [`LatestEventValue::Remote`].
pub type RemoteLatestEventValue = TimelineEvent;

Expand All @@ -55,6 +84,79 @@ pub struct LocalLatestEventValue {
pub content: SerializableEventContent,
}

#[cfg(test)]
mod tests_latest_event_value {
use ruma::{
MilliSecondsSinceUnixEpoch,
events::{AnyMessageLikeEventContent, room::message::RoomMessageEventContent},
serde::Raw,
uint,
};
use serde_json::json;

use super::{LatestEventValue, LocalLatestEventValue, RemoteLatestEventValue};
use crate::store::SerializableEventContent;

#[test]
fn test_timestamp_with_none() {
let value = LatestEventValue::None;

assert_eq!(value.timestamp(), None);
}

#[test]
fn test_timestamp_with_remote() {
let value = LatestEventValue::Remote(RemoteLatestEventValue::from_plaintext(
Raw::from_json_string(
json!({
"content": RoomMessageEventContent::text_plain("raclette"),
"type": "m.room.message",
"event_id": "$ev0",
"room_id": "!r0",
"origin_server_ts": 42,
"sender": "@mnt_io:matrix.org",
})
.to_string(),
)
.unwrap(),
));

assert_eq!(value.timestamp(), Some(42));
}

#[test]
fn test_timestamp_with_local_is_sending() {
let value = LatestEventValue::LocalIsSending(LocalLatestEventValue {
timestamp: MilliSecondsSinceUnixEpoch(uint!(42)),
content: SerializableEventContent::from_raw(
Raw::new(&AnyMessageLikeEventContent::RoomMessage(
RoomMessageEventContent::text_plain("raclette"),
))
.unwrap(),
"m.room.message".to_owned(),
),
});

assert_eq!(value.timestamp(), Some(42));
}

#[test]
fn test_timestamp_with_local_cannot_be_sent() {
let value = LatestEventValue::LocalCannotBeSent(LocalLatestEventValue {
timestamp: MilliSecondsSinceUnixEpoch(uint!(42)),
content: SerializableEventContent::from_raw(
Raw::new(&AnyMessageLikeEventContent::RoomMessage(
RoomMessageEventContent::text_plain("raclette"),
))
.unwrap(),
"m.room.message".to_owned(),
),
});

assert_eq!(value.timestamp(), Some(42));
}
}

/// Represents a decision about whether an event could be stored as the latest
/// event in a room. Variants starting with Yes indicate that this message could
/// be stored, and provide the inner event information, and those starting with
Expand Down
4 changes: 2 additions & 2 deletions crates/matrix-sdk-base/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ pub use once_cell;
pub use room::{
EncryptionState, InviteAcceptanceDetails, PredecessorRoom, Room,
RoomCreateWithCreatorEventContent, RoomDisplayName, RoomHero, RoomInfo, RoomInfoNotableUpdate,
RoomInfoNotableUpdateReasons, RoomMember, RoomMembersUpdate, RoomMemberships, RoomState,
RoomStateFilter, SuccessorRoom, apply_redaction,
RoomInfoNotableUpdateReasons, RoomMember, RoomMembersUpdate, RoomMemberships, RoomRecencyStamp,
RoomState, RoomStateFilter, SuccessorRoom, apply_redaction,
};
pub use store::{
ComposerDraft, ComposerDraftType, QueueWedgeError, StateChanges, StateStore, StateStoreDataKey,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ fn properties(
}

if let Some(recency_stamp) = &room_response.bump_stamp {
let recency_stamp: u64 = (*recency_stamp).into();
let recency_stamp = u64::from(*recency_stamp).into();

if room_info.recency_stamp.as_ref() != Some(&recency_stamp) {
room_info.update_recency_stamp(recency_stamp);
Expand Down
4 changes: 2 additions & 2 deletions crates/matrix-sdk-base/src/room/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ pub use members::{RoomMember, RoomMembersUpdate, RoomMemberships};
pub(crate) use room_info::SyncInfo;
pub use room_info::{
BaseRoomInfo, InviteAcceptanceDetails, RoomInfo, RoomInfoNotableUpdate,
RoomInfoNotableUpdateReasons, apply_redaction,
RoomInfoNotableUpdateReasons, RoomRecencyStamp, apply_redaction,
};
use ruma::{
EventId, OwnedEventId, OwnedMxcUri, OwnedRoomAliasId, OwnedRoomId, OwnedUserId, RoomId,
Expand Down Expand Up @@ -479,7 +479,7 @@ impl Room {
/// Returns the recency stamp of the room.
///
/// Please read `RoomInfo::recency_stamp` to learn more.
pub fn recency_stamp(&self) -> Option<u64> {
pub fn recency_stamp(&self) -> Option<RoomRecencyStamp> {
self.inner.read().recency_stamp
}

Expand Down
46 changes: 37 additions & 9 deletions crates/matrix-sdk-base/src/room/room_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -490,12 +490,22 @@ pub struct RoomInfo {

/// The recency stamp of this room.
///
/// It's not to be confused with `origin_server_ts` of the latest event.
/// Sliding Sync might "ignore” some events when computing the recency
/// stamp of the room. Thus, using this `recency_stamp` value is
/// more accurate than relying on the latest event.
/// It's not to be confused with the `origin_server_ts` value of an event.
/// Sliding Sync might “ignore” some events when computing the recency
/// stamp of the room. The recency stamp must be considered as an opaque
/// unsigned integer value.
///
/// # Sorting rooms
///
/// The recency stamp is designed to _sort_ rooms between them. The room
/// with the highest stamp should be at the top of a room list. However, in
/// some situation, it might be inaccurate (for example if the server and
/// the client disagree on which events should increment the recency stamp).
/// The [`LatestEventValue`] might be a useful alternative to sort rooms
/// between them as it's all computed client-side. In this case, the recency
/// stamp nicely acts as a default fallback.
#[serde(default)]
pub(crate) recency_stamp: Option<u64>,
pub(crate) recency_stamp: Option<RoomRecencyStamp>,

/// A timestamp remembering when we observed the user accepting an invite on
/// this current device.
Expand Down Expand Up @@ -1050,8 +1060,8 @@ impl RoomInfo {

/// Updates the recency stamp of this room.
///
/// Please read [`Self::recency_stamp`] to learn more.
pub(crate) fn update_recency_stamp(&mut self, stamp: u64) {
/// Please read `Self::recency_stamp` to learn more.
pub fn update_recency_stamp(&mut self, stamp: RoomRecencyStamp) {
self.recency_stamp = Some(stamp);
}

Expand Down Expand Up @@ -1136,6 +1146,24 @@ impl RoomInfo {
}
}

/// Type to represent a `RoomInfo::recency_stamp`.
#[repr(transparent)]
#[derive(Clone, Copy, Debug, Serialize, Deserialize, PartialEq)]
#[serde(transparent)]
pub struct RoomRecencyStamp(u64);

impl From<u64> for RoomRecencyStamp {
fn from(value: u64) -> Self {
Self(value)
}
}

impl From<RoomRecencyStamp> for u64 {
fn from(value: RoomRecencyStamp) -> Self {
value.0
}
}

#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
pub(crate) enum SyncInfo {
/// We only know the room exists and whether it is in invite / joined / left
Expand Down Expand Up @@ -1313,7 +1341,7 @@ mod tests {
warned_about_unknown_room_version_rules: Arc::new(false.into()),
cached_display_name: None,
cached_user_defined_notification_mode: None,
recency_stamp: Some(42),
recency_stamp: Some(42.into()),
invite_acceptance_details: None,
};

Expand Down Expand Up @@ -1562,7 +1590,7 @@ mod tests {
info.cached_user_defined_notification_mode.as_ref(),
Some(&RoomNotificationMode::Mute)
);
assert_eq!(info.recency_stamp.as_ref(), Some(&42));
assert_eq!(info.recency_stamp.as_ref(), Some(&42.into()));
}

// Ensure we can still deserialize RoomInfos before we added things to its
Expand Down
8 changes: 4 additions & 4 deletions crates/matrix-sdk-base/src/sliding_sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1794,7 +1794,7 @@ mod tests {

// Then the room in the client has the recency stamp
let client_room = client.get_room(room_id).expect("No room found");
assert_eq!(client_room.recency_stamp().expect("No recency stamp"), 42);
assert_eq!(client_room.recency_stamp().expect("No recency stamp"), 42.into());
}

#[async_test]
Expand All @@ -1816,7 +1816,7 @@ mod tests {

// Then the room in the client has the recency stamp
let client_room = client.get_room(room_id).expect("No room found");
assert_eq!(client_room.recency_stamp().expect("No recency stamp"), 42);
assert_eq!(client_room.recency_stamp().expect("No recency stamp"), 42.into());
}

{
Expand All @@ -1832,7 +1832,7 @@ mod tests {

// Then the room in the client has the previous recency stamp
let client_room = client.get_room(room_id).expect("No room found");
assert_eq!(client_room.recency_stamp().expect("No recency stamp"), 42);
assert_eq!(client_room.recency_stamp().expect("No recency stamp"), 42.into());
}

{
Expand All @@ -1849,7 +1849,7 @@ mod tests {

// Then the room in the client has the recency stamp
let client_room = client.get_room(room_id).expect("No room found");
assert_eq!(client_room.recency_stamp().expect("No recency stamp"), 153);
assert_eq!(client_room.recency_stamp().expect("No recency stamp"), 153.into());
}
}

Expand Down
65 changes: 27 additions & 38 deletions crates/matrix-sdk-ui/src/room_list_service/filters/category.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,47 +32,36 @@ pub enum RoomCategory {

type DirectTargetsLength = usize;

struct CategoryRoomMatcher<F>
/// _Direct targets_ mean the number of users in a direct room, except us.
/// So if it returns 1, it means there are 2 users in the direct room.
fn matches<F>(number_of_direct_targets: F, room: &Room, expected_kind: RoomCategory) -> bool
where
F: Fn(&Room) -> Option<DirectTargetsLength>,
{
/// _Direct targets_ mean the number of users in a direct room, except us.
/// So if it returns 1, it means there are 2 users in the direct room.
number_of_direct_targets: F,
}
let kind = match number_of_direct_targets(room) {
// If 1, we are sure it's a direct room between two users. It's the strict
// definition of the `People` category, all good.
Some(1) => RoomCategory::People,

// If smaller than 1, we are not sure it's a direct room, it's then a `Group`.
// If greater than 1, we are sure it's a direct room but not between
// two users, so it's a `Group` based on our expectation.
Some(_) => RoomCategory::Group,

// Don't know.
None => return false,
};

impl<F> CategoryRoomMatcher<F>
where
F: Fn(&Room) -> Option<DirectTargetsLength>,
{
fn matches(&self, room: &Room, expected_kind: RoomCategory) -> bool {
let kind = match (self.number_of_direct_targets)(room) {
// If 1, we are sure it's a direct room between two users. It's the strict
// definition of the `People` category, all good.
Some(1) => RoomCategory::People,

// If smaller than 1, we are not sure it's a direct room, it's then a `Group`.
// If greater than 1, we are sure it's a direct room but not between
// two users, so it's a `Group` based on our expectation.
Some(_) => RoomCategory::Group,

// Don't know.
None => return false,
};

kind == expected_kind
}
kind == expected_kind
}

/// Create a new filter that will accept all rooms that fit in the
/// `expected_category`. The category is defined by [`RoomCategory`], see this
/// type to learn more.
pub fn new_filter(expected_category: RoomCategory) -> impl Filter {
let matcher = CategoryRoomMatcher {
number_of_direct_targets: move |room| Some(room.direct_targets_length()),
};
let number_of_direct_targets = move |room: &Room| Some(room.direct_targets_length());

move |room| -> bool { matcher.matches(room, expected_category) }
move |room| -> bool { matches(number_of_direct_targets, room, expected_category) }
}

#[cfg(test)]
Expand All @@ -90,20 +79,20 @@ mod tests {
let (client, server) = logged_in_client_with_server().await;
let [room] = new_rooms([room_id!("!a:b.c")], &client, &server).await;

let matcher = CategoryRoomMatcher { number_of_direct_targets: |_| Some(42) };
let number_of_direct_targets = |_room: &Room| Some(42);

// Expect `People`.
{
let expected_kind = RoomCategory::People;

assert!(matcher.matches(&room, expected_kind).not());
assert!(matches(number_of_direct_targets, &room, expected_kind).not());
}

// Expect `Group`.
{
let expected_kind = RoomCategory::Group;

assert!(matcher.matches(&room, expected_kind));
assert!(matches(number_of_direct_targets, &room, expected_kind));
}
}

Expand All @@ -112,20 +101,20 @@ mod tests {
let (client, server) = logged_in_client_with_server().await;
let [room] = new_rooms([room_id!("!a:b.c")], &client, &server).await;

let matcher = CategoryRoomMatcher { number_of_direct_targets: |_| Some(1) };
let number_of_direct_targets = |_room: &Room| Some(1);

// Expect `People`.
{
let expected_kind = RoomCategory::People;

assert!(matcher.matches(&room, expected_kind));
assert!(matches(number_of_direct_targets, &room, expected_kind));
}

// Expect `Group`.
{
let expected_kind = RoomCategory::Group;

assert!(matcher.matches(&room, expected_kind).not());
assert!(matches(number_of_direct_targets, &room, expected_kind).not());
}
}

Expand All @@ -134,8 +123,8 @@ mod tests {
let (client, server) = logged_in_client_with_server().await;
let [room] = new_rooms([room_id!("!a:b.c")], &client, &server).await;

let matcher = CategoryRoomMatcher { number_of_direct_targets: |_| None };
let number_of_direct_targets = |_room: &Room| None;

assert!(matcher.matches(&room, RoomCategory::Group).not());
assert!(matches(number_of_direct_targets, &room, RoomCategory::Group).not());
}
}
Loading
Loading