diff --git a/matrix_sdk_base/src/client.rs b/matrix_sdk_base/src/client.rs index 5b9db3e4..b7b4be8f 100644 --- a/matrix_sdk_base/src/client.rs +++ b/matrix_sdk_base/src/client.rs @@ -1159,24 +1159,24 @@ impl BaseClient { match &*olm { Some(o) => { - let (history_visiblity, settings) = self + let (history_visibility, settings) = self .get_room(room_id) - .map(|r| (r.history_visiblity(), r.encryption_settings())) + .map(|r| (r.history_visibility(), r.encryption_settings())) .unwrap_or((HistoryVisibility::Joined, None)); let joined = self.store.get_joined_user_ids(room_id).await?; let invited = self.store.get_invited_user_ids(room_id).await?; // Don't share the group session with members that are invited - // if the history visiblity is set to `Joined` - let members = if history_visiblity == HistoryVisibility::Joined { + // if the history visibility is set to `Joined` + let members = if history_visibility == HistoryVisibility::Joined { joined.iter().chain(&[]) } else { joined.iter().chain(&invited) }; let settings = settings.ok_or(MegolmError::EncryptionNotEnabled)?; - let settings = EncryptionSettings::new(settings, history_visiblity); + let settings = EncryptionSettings::new(settings, history_visibility); Ok(o.share_group_session(room_id, members, settings).await?) } diff --git a/matrix_sdk_base/src/rooms/mod.rs b/matrix_sdk_base/src/rooms/mod.rs index 335139d0..5186da5f 100644 --- a/matrix_sdk_base/src/rooms/mod.rs +++ b/matrix_sdk_base/src/rooms/mod.rs @@ -80,8 +80,8 @@ impl RoomState { } } - /// Get the history visiblity policy of this room. - pub fn history_visiblity(&self) -> HistoryVisibility { + /// Get the history visibility policy of this room. + pub fn history_visibility(&self) -> HistoryVisibility { match self { RoomState::Joined(r) => r.inner.history_visibility(), RoomState::Left(r) => r.inner.history_visibility(), @@ -169,7 +169,7 @@ pub struct BaseRoomInfo { pub encryption: Option, /// The guest access policy of this room. pub guest_access: GuestAccess, - /// The history visiblity policy of this room. + /// The history visibility policy of this room. pub history_visibility: HistoryVisibility, /// The join rule policy of this room. pub join_rule: JoinRule, diff --git a/matrix_sdk_base/src/rooms/normal.rs b/matrix_sdk_base/src/rooms/normal.rs index da7777fb..2220b7f9 100644 --- a/matrix_sdk_base/src/rooms/normal.rs +++ b/matrix_sdk_base/src/rooms/normal.rs @@ -197,7 +197,7 @@ impl Room { self.inner.read().unwrap().base_info.guest_access.clone() } - /// Get the history visiblity policy of this room. + /// Get the history visibility policy of this room. pub fn history_visibility(&self) -> HistoryVisibility { self.inner .read() diff --git a/matrix_sdk_base/src/rooms/stripped.rs b/matrix_sdk_base/src/rooms/stripped.rs index 35ba4fc0..8b94a7af 100644 --- a/matrix_sdk_base/src/rooms/stripped.rs +++ b/matrix_sdk_base/src/rooms/stripped.rs @@ -105,7 +105,7 @@ impl StrippedRoom { self.inner.lock().unwrap().base_info.encryption.clone() } - /// Get the history visiblity policy of this room. + /// Get the history visibility policy of this room. pub fn history_visibility(&self) -> HistoryVisibility { self.inner .lock() diff --git a/matrix_sdk_crypto/src/olm/account.rs b/matrix_sdk_crypto/src/olm/account.rs index d39532ad..46e59d4b 100644 --- a/matrix_sdk_crypto/src/olm/account.rs +++ b/matrix_sdk_crypto/src/olm/account.rs @@ -991,7 +991,7 @@ impl ReadOnlyAccount { return Err(()); } - let visiblity = settings.history_visibility.clone(); + let visibility = settings.history_visibility.clone(); let outbound = OutboundGroupSession::new( self.device_id.clone(), @@ -1009,7 +1009,7 @@ impl ReadOnlyAccount { signing_key, &room_id, outbound.session_key().await, - Some(visiblity), + Some(visibility), ) .expect("Can't create inbound group session from a newly created outbound group session"); diff --git a/matrix_sdk_crypto/src/olm/group_sessions/inbound.rs b/matrix_sdk_crypto/src/olm/group_sessions/inbound.rs index b39cfc64..db373133 100644 --- a/matrix_sdk_crypto/src/olm/group_sessions/inbound.rs +++ b/matrix_sdk_crypto/src/olm/group_sessions/inbound.rs @@ -383,7 +383,7 @@ pub struct PickledInboundGroupSession { /// Flag remembering if the session was dirrectly sent to us by the sender /// or if it was imported. pub imported: bool, - /// History visiblity of the room when the session was created. + /// History visibility of the room when the session was created. pub history_visibility: Option, } diff --git a/matrix_sdk_crypto/src/olm/group_sessions/outbound.rs b/matrix_sdk_crypto/src/olm/group_sessions/outbound.rs index 49aff184..449b64e8 100644 --- a/matrix_sdk_crypto/src/olm/group_sessions/outbound.rs +++ b/matrix_sdk_crypto/src/olm/group_sessions/outbound.rs @@ -98,7 +98,7 @@ impl Default for EncryptionSettings { impl EncryptionSettings { /// Create new encryption settings using an `EncryptionEventContent` and a - /// history visiblity. + /// history visibility. pub fn new(content: EncryptionEventContent, history_visibility: HistoryVisibility) -> Self { let rotation_period: Duration = content .rotation_period_ms diff --git a/matrix_sdk_crypto/src/session_manager/group_sessions.rs b/matrix_sdk_crypto/src/session_manager/group_sessions.rs index eac5369b..afe00ab4 100644 --- a/matrix_sdk_crypto/src/session_manager/group_sessions.rs +++ b/matrix_sdk_crypto/src/session_manager/group_sessions.rs @@ -213,10 +213,10 @@ impl GroupSessionManager { Ok((id, request, changed_sessions)) } - /// Given a list of user and an outbound session get the list of users and - /// devices that this session should be shared with. + /// Given a list of user and an outbound session, return the list of users + /// and their devices that this session should be shared with. /// - /// Returns a boolean indicating that the session needs to be rotated and + /// Returns a boolean indicating whether the session needs to be rotated and /// the list of users/devices that should receive the session. pub async fn collect_session_recipients( &self, @@ -236,32 +236,39 @@ impl GroupSessionManager { let users_shared_with: HashSet<&UserId> = users_shared_with.iter().collect(); // A user left if a user is missing from the set of users that should - // get the session but is in the set of users that received the sessoin. + // get the session but is in the set of users that received the session. let user_left = !users_shared_with .difference(&users) .collect::>() .is_empty(); - let visiblity_changed = outbound.settings().history_visibility != history_visibility; + let visibility_changed = outbound.settings().history_visibility != history_visibility; - let mut device_got_deleted_or_blacklisted = false; + // To protect the room history we need to rotate the session if either: + // + // 1. Any user left the room. + // 2. Any of the users' devices got deleted or blacklisted. + // 3. The history visibility changed. + // + // This is calculated in the following code and stored in this variable. + let mut should_rotate = user_left || visibility_changed; for user_id in users { let user_devices = self.store.get_user_devices(&user_id).await?; + let non_blacklisted_devices: Vec = user_devices + .devices() + .filter(|d| !d.is_blacklisted()) + .collect(); - // If no device got deleted or blacklisted until now and no user - // left, check if one got deleted or blacklisted for this user. - if !(device_got_deleted_or_blacklisted || user_left || visiblity_changed) { - // Devices that should receive this session - let device_ids: HashSet<&DeviceId> = user_devices - .keys() - .filter(|d| { - !user_devices - .get(d) - .map(|d| d.is_blacklisted()) - .unwrap_or(false) - }) - .map(|d| d.as_ref()) + // If we haven't already concluded that the session should be + // rotated for other reasons, we also need to check whether any + // of the devices in the session got deleted or blacklisted in the + // meantime. If so, we should also rotate the session. + if !should_rotate { + // Device IDs that should receive this session + let non_blacklisted_device_ids: HashSet<&DeviceId> = non_blacklisted_devices + .iter() + .map(|d| d.device_id()) .collect(); if let Some(shared) = outbound.shared_with_set.get(user_id) { @@ -271,18 +278,19 @@ impl GroupSessionManager { shared.iter().map(|d| d.key().clone()).collect(); let shared: HashSet<&DeviceId> = shared.iter().map(|d| d.as_ref()).collect(); - // The difference between the devices that received the - // session and devices that should receive the session are - // our deleted or newly blacklisted devices + // The set difference between // - // If the set isn't empty, a device got blacklisted or - // deleted, so remember it. - if !shared - .difference(&device_ids) - .collect::>() - .is_empty() - { - device_got_deleted_or_blacklisted = true; + // 1. Devices that had previously received the session, and + // 2. Devices that would now receive the session + // + // represents newly deleted or blacklisted devices. If this + // set is non-empty, we must rotate. + let newly_deleted_or_blacklisted = shared + .difference(&non_blacklisted_device_ids) + .collect::>(); + + if !newly_deleted_or_blacklisted.is_empty() { + should_rotate = true; } }; } @@ -290,15 +298,9 @@ impl GroupSessionManager { devices .entry(user_id.clone()) .or_insert_with(Vec::new) - .extend(user_devices.devices().filter(|d| !d.is_blacklisted())); + .extend(non_blacklisted_devices); } - // To protect the room history we need to rotate the session if a user - // left or if a device got deleted/blacklisted, put differently if - // someone leaves or gets removed from the encrypted group or if the - // history visiblity changed. - let should_rotate = user_left || device_got_deleted_or_blacklisted || visiblity_changed; - Ok((should_rotate, devices)) } @@ -341,6 +343,7 @@ impl GroupSessionManager { changes.inbound_group_sessions.push(inbound); debug!( + room_id = room_id.as_str(), "A user/device has left the group {} since we last sent a message, \ rotating the outbound session.", room_id @@ -368,15 +371,19 @@ impl GroupSessionManager { let message_index = outbound.message_index().await; if !devices.is_empty() { + let users = devices.iter().fold(BTreeMap::new(), |mut acc, d| { + acc.entry(d.user_id()) + .or_insert_with(BTreeSet::new) + .insert(d.device_id()); + acc + }); + info!( - "Sharing outbound session at index {} with {:?}", + index = message_index, + users = ?users, + "Sharing an outbound session at index {} with {:?}", message_index, - devices.iter().fold(BTreeMap::new(), |mut acc, d| { - acc.entry(d.user_id()) - .or_insert_with(BTreeSet::new) - .insert(d.device_id()); - acc - }) + users ); } @@ -398,6 +405,8 @@ impl GroupSessionManager { if requests.is_empty() { debug!( + room_id = room_id.as_str(), + session_id = outbound.session_id(), "Session {} for room {} doesn't need to be shared with anyone, marking as shared", outbound.session_id(), outbound.room_id()