From efd40b9457f62698b322ac4f6e07d9508259d25e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Klocek?= Date: Fri, 6 Dec 2024 10:14:13 +0100 Subject: [PATCH] [identity] Verify new flow migration device list update Summary: Address [[ https://linear.app/comm/issue/ENG-9694/oldnew-flow-based-verification-for-updatedevicelist-rpc | ENG-9694 ]] > - For old flows: Should allow reordering primary device under the following conditions: > - Previous device list was unsigned. > - Reorder only makes the calling device primary (only one swap, no more changes). > - Otherwise, if previous device list was signed, it should act as before Depends on D14087 Test Plan: - Commtest - Manual testing during development of device list migration handler Reviewers: kamil Reviewed By: kamil Subscribers: ashoat, tomek Differential Revision: https://phab.comm.dev/D14088 --- .../tests/identity_device_list_tests.rs | 59 +++++++++++++++++-- services/identity/src/device_list.rs | 16 +++++ .../src/grpc_services/authenticated.rs | 51 +++++++++++++++- 3 files changed, 117 insertions(+), 9 deletions(-) diff --git a/services/commtest/tests/identity_device_list_tests.rs b/services/commtest/tests/identity_device_list_tests.rs index d81ffe05ba..1c366e5943 100644 --- a/services/commtest/tests/identity_device_list_tests.rs +++ b/services/commtest/tests/identity_device_list_tests.rs @@ -97,7 +97,10 @@ async fn test_device_list_rotation() { #[tokio::test] async fn test_update_device_list_rpc() { // Register user with primary device - let primary_device = register_user_device(None, None).await; + let mut primary_account = MockOlmAccount::new(); + let primary_device_keys = primary_account.public_keys(); + let primary_device = + register_user_device(Some(&primary_device_keys), None).await; let mut auth_client = get_auth_client( &service_addr::IDENTITY_GRPC.to_string(), primary_device.user_id.clone(), @@ -120,6 +123,14 @@ async fn test_update_device_list_rpc() { assert!(initial_device_list.len() == 1, "Expected single device"); let primary_device_id = initial_device_list[0].clone(); + // migrate to signed device lists + migrate_device_list( + &mut auth_client, + &initial_device_list, + &mut primary_account, + ) + .await; + // perform update by adding a new device let now = SystemTime::now().duration_since(UNIX_EPOCH).unwrap(); let devices_payload = vec![primary_device_id, "device2".to_string()]; @@ -170,13 +181,32 @@ async fn test_device_list_signatures() { .await .expect("Couldn't connect to identity service"); - // Perform unsigned update (add a new device) + // perform a migration to signed device list + let migration_update: DeviceListHistoryItem = { + let latest_device_list = &[primary_device_id.to_string()]; + let updated_device_list = migrate_device_list( + &mut auth_client, + latest_device_list, + &mut primary_account, + ) + .await; + + ( + updated_device_list.cur_primary_signature.clone(), + updated_device_list.into_raw().devices, + ) + }; + + // Perform first update (add a new device) let first_update: DeviceListHistoryItem = { - let update_payload = - SignedDeviceList::from_raw_unsigned(&RawDeviceList::new(vec![ + let update_payload = SignedDeviceList::create_signed( + &RawDeviceList::new(vec![ primary_device_id.to_string(), "device2".to_string(), - ])); + ]), + &mut primary_account, + None, + ); let update_request = UpdateDeviceListRequest::from(&update_payload); auth_client .update_device_list(update_request) @@ -189,7 +219,7 @@ async fn test_device_list_signatures() { ) }; - // now perform a update (remove a device), but sign the device list + // now perform another update (remove a device) let second_update: DeviceListHistoryItem = { let update_payload = SignedDeviceList::create_signed( &RawDeviceList::new(vec![primary_device_id.to_string()]), @@ -238,6 +268,7 @@ async fn test_device_list_signatures() { let expected_devices_lists: Vec = vec![ (None, vec![primary_device_id.to_string()]), // auto-generated during registration + migration_update, first_update, second_update, ]; @@ -555,3 +586,19 @@ async fn get_raw_device_list_history( .map(|signed| signed.into_raw()) .collect() } + +async fn migrate_device_list( + client: &mut ChainedInterceptedAuthClient, + last_device_list: &[String], + signing_account: &mut MockOlmAccount, +) -> SignedDeviceList { + let raw_list = RawDeviceList::new(Vec::from(last_device_list)); + let signed_list = + SignedDeviceList::create_signed(&raw_list, signing_account, None); + client + .update_device_list(UpdateDeviceListRequest::from(&signed_list)) + .await + .expect("Failed to perform signed device list migration"); + + signed_list +} diff --git a/services/identity/src/device_list.rs b/services/identity/src/device_list.rs index 505d17d950..e8f97aed1b 100644 --- a/services/identity/src/device_list.rs +++ b/services/identity/src/device_list.rs @@ -382,6 +382,22 @@ pub mod validation { is_added != is_removed } + pub fn new_flow_migration_validator( + previous_device_list: &[&str], + new_device_list: &[&str], + calling_device_id: &str, + ) -> bool { + // new primary must be the calling device + if new_device_list.first() != Some(&calling_device_id) { + return false; + } + + // no device added or removed, only reorder allowed + let previous_set: HashSet<_> = previous_device_list.iter().collect(); + let new_set: HashSet<_> = new_device_list.iter().collect(); + previous_set == new_set + } + #[cfg(test)] mod tests { use super::*; diff --git a/services/identity/src/grpc_services/authenticated.rs b/services/identity/src/grpc_services/authenticated.rs index d0b893ec33..7cf5074bbc 100644 --- a/services/identity/src/grpc_services/authenticated.rs +++ b/services/identity/src/grpc_services/authenticated.rs @@ -920,13 +920,58 @@ impl IdentityClientService for AuthenticatedService { ) .await?; + let is_new_flow_user = self + .db_client + .get_user_login_flow(&user_id) + .await? + .is_signed_device_list_flow(); + let new_list = SignedDeviceList::try_from(request.into_inner())?; let update = DeviceListUpdate::try_from(new_list)?; - let validator = - crate::device_list::validation::update_device_list_rpc_validator; + + let validator = if is_new_flow_user { + // regular device list update + Some(crate::device_list::validation::update_device_list_rpc_validator) + } else { + // new flow migration + let Some(current_device_list) = + self.db_client.get_current_device_list(&user_id).await? + else { + tracing::warn!("User {} does not have valid device list. New flow migration impossible.", redact_sensitive_data(&user_id)); + return Err(tonic::Status::aborted( + tonic_status_messages::DEVICE_LIST_ERROR, + )); + }; + + let calling_device_id = &device_id; + let previous_device_ids: Vec<&str> = current_device_list + .device_ids + .iter() + .map(AsRef::as_ref) + .collect(); + let new_device_ids: Vec<&str> = + update.devices.iter().map(AsRef::as_ref).collect(); + + let is_valid = + crate::device_list::validation::new_flow_migration_validator( + &previous_device_ids, + &new_device_ids, + calling_device_id, + ); + if !is_valid { + return Err( + crate::error::Error::DeviceList( + crate::error::DeviceListError::InvalidDeviceListUpdate, + ) + .into(), + ); + } + // we've already validated it, no further validator required + None + }; self .db_client - .apply_devicelist_update(&user_id, update, Some(validator), true) + .apply_devicelist_update(&user_id, update, validator, true) .await?; Ok(Response::new(Empty {}))