Merge branch 'group-session-sharing-code-refactoring'

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`
+                // if the history visibility is set to `Joined`
-                let members = if history_visiblity == HistoryVisibility::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?)
             }

matrix_sdk_base/src/rooms/mod.rs

@@ -80,8 +80,8 @@ impl RoomState {
         }
     }
 
-    /// Get the history visiblity policy of this room.
+    /// Get the history visibility policy of this room.
-    pub fn history_visiblity(&self) -> HistoryVisibility {
+    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<EncryptionEventContent>,
     /// 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,

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()

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()

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");
 

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<HistoryVisibility>,
 }
 

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

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
+    /// Given a list of user and an outbound session, return the list of users
-    /// devices that this session should be shared with.
+    /// 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::<HashSet<_>>()
             .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<Device> = user_devices
+                .devices()
+                .filter(|d| !d.is_blacklisted())
+                .collect();
 
-            // If no device got deleted or blacklisted until now and no user
+            // If we haven't already concluded that the session should be
-            // left, check if one got deleted or blacklisted for this user.
+            // rotated for other reasons, we also need to check whether any
-            if !(device_got_deleted_or_blacklisted || user_left || visiblity_changed) {
+            // of the devices in the session got deleted or blacklisted in the
-                // Devices that should receive this session
+            // meantime. If so, we should also rotate the session.
-                let device_ids: HashSet<&DeviceId> = user_devices
+            if !should_rotate {
-                    .keys()
+                // Device IDs that should receive this session
-                    .filter(|d| {
+                let non_blacklisted_device_ids: HashSet<&DeviceId> = non_blacklisted_devices
-                        !user_devices
+                    .iter()
-                            .get(d)
+                    .map(|d| d.device_id())
-                            .map(|d| d.is_blacklisted())
-                            .unwrap_or(false)
-                    })
-                    .map(|d| d.as_ref())
                     .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
+                    // The set difference between
-                    // session and devices that should receive the session are
-                    // our deleted or newly blacklisted devices
                     //
-                    // If the set isn't empty, a device got blacklisted or
+                    // 1. Devices that had previously received the session, and
-                    // deleted, so remember it.
+                    // 2. Devices that would now receive the session
-                    if !shared
+                    //
-                        .difference(&device_ids)
+                    // represents newly deleted or blacklisted devices. If this
-                        .collect::<HashSet<_>>()
+                    // set is non-empty, we must rotate.
-                        .is_empty()
+                    let newly_deleted_or_blacklisted = shared
-                    {
+                        .difference(&non_blacklisted_device_ids)
-                        device_got_deleted_or_blacklisted = true;
+                        .collect::<HashSet<_>>();
+
+                    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() {
-            info!(
+            let users = devices.iter().fold(BTreeMap::new(), |mut acc, d| {
-                "Sharing 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
-                })
+            });
+
+            info!(
+                index = message_index,
+                users = ?users,
+                "Sharing an outbound session at index {} with {:?}",
+                message_index,
+                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()