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.

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

refactor collect_session_recipients in advance of some behavioural changes #3884

Merged
merged 2 commits into from
Aug 27, 2024
Merged

refactor collect_session_recipients in advance of some behavioural changes #3884

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c0db53
refactor collect_session_recipients in advance of some behavioural ch…
uhoreg Aug 22, 2024
7ce88fd
apply changes from review
uhoreg Aug 23, 2024
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
151 changes: 86 additions & 65 deletions crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,6 @@ pub(crate) async fn collect_session_recipients(
let users: BTreeSet<&UserId> = users.collect();
let mut devices: BTreeMap<OwnedUserId, Vec<DeviceData>> = Default::default();
let mut withheld_devices: Vec<(DeviceData, WithheldCode)> = Default::default();
let mut unsigned_devices_of_verified_users: BTreeMap<OwnedUserId, Vec<OwnedDeviceId>> =
Default::default();
let mut verified_users_with_new_identities: Vec<OwnedUserId> = Default::default();

trace!(?users, ?settings, "Calculating group session recipients");

Expand All @@ -148,17 +145,23 @@ pub(crate) async fn collect_session_recipients(
// This is calculated in the following code and stored in this variable.
let mut should_rotate = user_left || visibility_changed || algorithm_changed;

let own_identity = store.get_user_identity(store.user_id()).await?.and_then(|i| i.into_own());
// Get the recipient and withheld devices, based on the collection strategy.
match settings.sharing_strategy {
CollectStrategy::DeviceBasedStrategy {
only_allow_trusted_devices,
error_on_verified_user_problem,
} => {
let mut unsigned_devices_of_verified_users: BTreeMap<OwnedUserId, Vec<OwnedDeviceId>> =
Default::default();
let mut verified_users_with_new_identities: Vec<OwnedUserId> = Default::default();

for user_id in users {
trace!("Considering recipient devices for user {}", user_id);
let user_devices = store.get_device_data_for_user_filtered(user_id).await?;
let own_identity =
store.get_user_identity(store.user_id()).await?.and_then(|i| i.into_own());

for user_id in users {
trace!("Considering recipient devices for user {}", user_id);
let user_devices = store.get_device_data_for_user_filtered(user_id).await?;

let recipient_devices = match settings.sharing_strategy {
CollectStrategy::DeviceBasedStrategy {
only_allow_trusted_devices,
error_on_verified_user_problem,
} => {
// We only need the user identity if `only_allow_trusted_devices` or
// `error_on_verified_user_problem` is set.
let device_owner_identity =
Expand All @@ -179,73 +182,91 @@ pub(crate) async fn collect_session_recipients(
continue;
}

split_devices_for_user(
let recipient_devices = split_devices_for_user(
user_devices,
&own_identity,
&device_owner_identity,
only_allow_trusted_devices,
error_on_verified_user_problem,
)
);

// If `error_on_verified_user_problem` is set, then
// `unsigned_of_verified_user` may be populated. If so, add an entry to the
// list of users with unsigned devices.
if !recipient_devices.unsigned_of_verified_user.is_empty() {
unsigned_devices_of_verified_users.insert(
user_id.to_owned(),
recipient_devices
.unsigned_of_verified_user
.into_iter()
.map(|d| d.device_id().to_owned())
.collect(),
);
}

let recipients = recipient_devices.allowed_devices;
let withheld_recipients = recipient_devices.denied_devices_with_code;

// 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 {
should_rotate = is_session_overshared_for_user(outbound, user_id, &recipients)
}

devices.entry(user_id.to_owned()).or_default().extend(recipients);
withheld_devices.extend(withheld_recipients);
}
CollectStrategy::IdentityBasedStrategy => {
let device_owner_identity = store.get_user_identity(user_id).await?;
split_recipients_withhelds_for_user_based_on_identity(
user_devices,
&device_owner_identity,
)

// If `error_on_verified_user_problem` is set, then
// `unsigned_devices_of_verified_users` may be populated. If so, we need to bail
// out with an error.
if !unsigned_devices_of_verified_users.is_empty() {
return Err(OlmError::SessionRecipientCollectionError(
SessionRecipientCollectionError::VerifiedUserHasUnsignedDevice(
unsigned_devices_of_verified_users,
),
));
}
};

// If we're using a `DeviceBasedStrategy` with
// `error_on_verified_user_problem` set, then
// `unsigned_of_verified_user` may be populated. If so, add an entry to the
// list of users with unsigned devices.
if !recipient_devices.unsigned_of_verified_user.is_empty() {
unsigned_devices_of_verified_users.insert(
user_id.to_owned(),
recipient_devices
.unsigned_of_verified_user
.into_iter()
.map(|d| d.device_id().to_owned())
.collect(),
);
// Alternatively, we may have encountered previously-verified users who have
// changed their identities. We bail out for that, too.
if !verified_users_with_new_identities.is_empty() {
return Err(OlmError::SessionRecipientCollectionError(
SessionRecipientCollectionError::VerifiedUserChangedIdentity(
verified_users_with_new_identities,
),
));
}
}
CollectStrategy::IdentityBasedStrategy => {
for user_id in users {
trace!("Considering recipient devices for user {}", user_id);
let user_devices = store.get_device_data_for_user_filtered(user_id).await?;

let recipients = recipient_devices.allowed_devices;
let withheld_recipients = recipient_devices.denied_devices_with_code;
let device_owner_identity = store.get_user_identity(user_id).await?;

// 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 {
should_rotate = is_session_overshared_for_user(outbound, user_id, &recipients)
}
let recipient_devices = split_recipients_withhelds_for_user_based_on_identity(
user_devices,
&device_owner_identity,
);

devices.entry(user_id.to_owned()).or_default().extend(recipients);
withheld_devices.extend(withheld_recipients);
}
let recipients = recipient_devices.allowed_devices;
let withheld_recipients = recipient_devices.denied_devices_with_code;
Copy link
Member

Choose a reason for hiding this comment

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

consider inlining withheld_recipients

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, this was done in #3662 (along with inlining recipients) so I've added that here now.


// If we're using a `DeviceBasedStrategy` with
// `error_on_verified_user_problem` set, then
// `unsigned_devices_of_verified_users` may be populated. If so, we need to bail
// out with an error.
if !unsigned_devices_of_verified_users.is_empty() {
return Err(OlmError::SessionRecipientCollectionError(
SessionRecipientCollectionError::VerifiedUserHasUnsignedDevice(
unsigned_devices_of_verified_users,
),
));
}
// 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 {
should_rotate = is_session_overshared_for_user(outbound, user_id, &recipients)
}

// Alternatively, we may have encountered previously-verified users who have
// changed their identities. We bail out for that, too.
if !verified_users_with_new_identities.is_empty() {
return Err(OlmError::SessionRecipientCollectionError(
SessionRecipientCollectionError::VerifiedUserChangedIdentity(
verified_users_with_new_identities,
),
));
devices.entry(user_id.to_owned()).or_default().extend(recipients);
withheld_devices.extend(withheld_recipients);
}
}
}

if should_rotate {
Expand Down
Loading