From b1b1ed346550e58d2f2c7b03295be21ef4a179fb Mon Sep 17 00:00:00 2001 From: Stephane Raux Date: Tue, 23 Jan 2024 19:33:07 -0600 Subject: [PATCH 1/3] Test that rejected proposals are proposals in cache and not in commit when receiving commit The test currently fails and the implementation will be fixed in the next commit. --- mls-rs/src/group/proposal_cache.rs | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/mls-rs/src/group/proposal_cache.rs b/mls-rs/src/group/proposal_cache.rs index 26f6d8d3..2644b389 100644 --- a/mls-rs/src/group/proposal_cache.rs +++ b/mls-rs/src/group/proposal_cache.rs @@ -4172,4 +4172,31 @@ mod tests { leaf_node: update_leaf_node(name, leaf_index).await, } } + + #[maybe_async::test(not(mls_build_async), async(mls_build_async, crate::futures_test))] + async fn when_receiving_commit_unused_proposals_are_proposals_in_cache_but_not_in_commit() { + let (alice, tree) = new_tree("alice").await; + + let proposal = Proposal::GroupContextExtensions(Default::default()); + let proposal_ref = make_proposal_ref(&proposal, alice).await; + + let state = CommitReceiver::new( + &tree, + alice, + alice, + test_cipher_suite_provider(TEST_CIPHER_SUITE), + ) + .cache(proposal_ref.clone(), proposal, alice) + .receive([Proposal::Add(Box::new(AddProposal { + key_package: test_key_package(TEST_PROTOCOL_VERSION, TEST_CIPHER_SUITE, "bob").await, + }))]) + .await + .unwrap(); + + let [p] = &state.rejected_proposals[..] else { + panic!("Expected single rejected proposal"); + }; + + assert_eq!(p.proposal_ref(), Some(&proposal_ref)); + } } From 5094fd7d75c0dff6e2e98c3b6c34c34cf4a52f0d Mon Sep 17 00:00:00 2001 From: Stephane Raux Date: Tue, 23 Jan 2024 20:37:06 -0600 Subject: [PATCH 2/3] Fix unused proposals when receiving commit Unused proposals were previously made of the proposals in the received commit that would cause the commit to be invalid and thus were excluded. But given that invalid proposals when receiving a commit cause the entire commit to be rejected, it means that unused proposals were always empty when receiving a commit. Unused proposals are now made of the proposals in the receiver's cache that are not in the received commit. --- mls-rs/src/group/proposal_cache.rs | 51 +++++++++++++--------- mls-rs/src/group/proposal_filter/bundle.rs | 43 +++++++++++++++++- 2 files changed, 72 insertions(+), 22 deletions(-) diff --git a/mls-rs/src/group/proposal_cache.rs b/mls-rs/src/group/proposal_cache.rs index 2644b389..450d564b 100644 --- a/mls-rs/src/group/proposal_cache.rs +++ b/mls-rs/src/group/proposal_cache.rs @@ -24,8 +24,8 @@ use mls_rs_core::{error::IntoAnyError, psk::PreSharedKeyStorage}; #[cfg(feature = "by_ref_proposal")] #[derive(Debug, Clone, MlsSize, MlsEncode, MlsDecode, PartialEq)] pub struct CachedProposal { - proposal: Proposal, - sender: Sender, + pub(crate) proposal: Proposal, + pub(crate) sender: Sender, } #[cfg(feature = "by_ref_proposal")] @@ -89,21 +89,21 @@ impl ProposalCache { sender: Sender, additional_proposals: Vec, ) -> ProposalBundle { - let mut proposals = ProposalBundle::default(); - - for (r, p) in &self.proposals { - proposals.add( - p.proposal.clone(), - p.sender, - ProposalSource::ByReference(r.clone()), - ); - } - - for p in additional_proposals.into_iter() { - proposals.add(p, sender, ProposalSource::ByValue); - } - - proposals + self.proposals + .iter() + .map(|(r, p)| { + ( + p.proposal.clone(), + p.sender, + ProposalSource::ByReference(r.clone()), + ) + }) + .chain( + additional_proposals + .into_iter() + .map(|p| (p, sender, ProposalSource::ByValue)), + ) + .collect() } pub fn resolve_for_commit( @@ -249,8 +249,13 @@ impl GroupState { .await?; #[cfg(feature = "by_ref_proposal")] - let rejected_proposals = - rejected_proposals(all_proposals, &applier_output.applied_proposals); + let rejected_proposals = rejected_proposals( + match direction { + CommitDirection::Send => all_proposals, + CommitDirection::Receive => self.proposals.proposals.iter().collect(), + }, + &applier_output.applied_proposals, + ); let mut group_context = self.context.clone(); group_context.epoch += 1; @@ -508,13 +513,14 @@ pub(crate) mod test_utils { context.extensions = group_extensions.clone(); - let state = GroupState::new( + let mut state = GroupState::new( context, public_tree.clone(), Vec::new().into(), ConfirmationTag::empty(cipher_suite_provider).await, ); + state.proposals.proposals = self.proposals.clone(); let proposals = self.resolve_for_commit(sender, proposal_list)?; state @@ -4194,7 +4200,10 @@ mod tests { .unwrap(); let [p] = &state.rejected_proposals[..] else { - panic!("Expected single rejected proposal"); + panic!( + "Expected single rejected proposal but got {:?}", + state.rejected_proposals + ); }; assert_eq!(p.proposal_ref(), Some(&proposal_ref)); diff --git a/mls-rs/src/group/proposal_filter/bundle.rs b/mls-rs/src/group/proposal_filter/bundle.rs index a1783625..5f091cbb 100644 --- a/mls-rs/src/group/proposal_filter/bundle.rs +++ b/mls-rs/src/group/proposal_filter/bundle.rs @@ -17,7 +17,7 @@ use crate::{ }; #[cfg(feature = "by_ref_proposal")] -use crate::group::{LeafIndex, ProposalRef, UpdateProposal}; +use crate::group::{proposal_cache::CachedProposal, LeafIndex, ProposalRef, UpdateProposal}; #[cfg(feature = "psk")] use crate::group::PreSharedKeyProposal; @@ -434,6 +434,47 @@ impl ProposalBundle { } } +impl FromIterator<(Proposal, Sender, ProposalSource)> for ProposalBundle { + fn from_iter(iter: I) -> Self + where + I: IntoIterator, + { + let mut bundle = ProposalBundle::default(); + for (proposal, sender, source) in iter { + bundle.add(proposal, sender, source); + } + bundle + } +} + +#[cfg(feature = "by_ref_proposal")] +impl<'a> FromIterator<(&'a ProposalRef, &'a CachedProposal)> for ProposalBundle { + fn from_iter(iter: I) -> Self + where + I: IntoIterator, + { + iter.into_iter() + .map(|(r, p)| { + ( + p.proposal.clone(), + p.sender, + ProposalSource::ByReference(r.clone()), + ) + }) + .collect() + } +} + +#[cfg(feature = "by_ref_proposal")] +impl<'a> FromIterator<&'a (ProposalRef, CachedProposal)> for ProposalBundle { + fn from_iter(iter: I) -> Self + where + I: IntoIterator, + { + iter.into_iter().map(|pair| (&pair.0, &pair.1)).collect() + } +} + #[cfg_attr( all(feature = "ffi", not(test)), safer_ffi_gen::ffi_type(clone, opaque) From 5134f40f743e007fd3a659be672e6c9921121582 Mon Sep 17 00:00:00 2001 From: Stephane Raux Date: Wed, 24 Jan 2024 10:34:14 -0600 Subject: [PATCH 3/3] Rename rejected_proposals to unused_proposals The previous term was confusing in the context of receiving a commit. This also removes outdated comments in tests from back when only unused proposals from the committer were returned. --- mls-rs/src/group/commit.rs | 2 +- mls-rs/src/group/message_processor.rs | 4 +- mls-rs/src/group/proposal_cache.rs | 118 ++++++++------------------ mls-rs/src/tree_kem/update_path.rs | 2 +- 4 files changed, 39 insertions(+), 87 deletions(-) diff --git a/mls-rs/src/group/commit.rs b/mls-rs/src/group/commit.rs index f01fb31c..5eb545e4 100644 --- a/mls-rs/src/group/commit.rs +++ b/mls-rs/src/group/commit.rs @@ -740,7 +740,7 @@ where ratchet_tree, external_commit_group_info, #[cfg(feature = "by_ref_proposal")] - unused_proposals: provisional_state.rejected_proposals, + unused_proposals: provisional_state.unused_proposals, }) } diff --git a/mls-rs/src/group/message_processor.rs b/mls-rs/src/group/message_processor.rs index 1b287b90..36766e04 100644 --- a/mls-rs/src/group/message_processor.rs +++ b/mls-rs/src/group/message_processor.rs @@ -73,7 +73,7 @@ pub(crate) struct ProvisionalState { pub(crate) external_init_index: Option, pub(crate) indexes_of_added_kpkgs: Vec, #[cfg(feature = "by_ref_proposal")] - pub(crate) rejected_proposals: Vec>, + pub(crate) unused_proposals: Vec>, } //By default, the path field of a Commit MUST be populated. The path field MAY be omitted if @@ -607,7 +607,7 @@ pub(crate) trait MessageProcessor: Send + Sync { #[cfg(feature = "custom_proposal")] custom_proposals: provisional.applied_proposals.custom_proposals.clone(), #[cfg(feature = "by_ref_proposal")] - unused_proposals: provisional.rejected_proposals.clone(), + unused_proposals: provisional.unused_proposals.clone(), }; Ok(update) diff --git a/mls-rs/src/group/proposal_cache.rs b/mls-rs/src/group/proposal_cache.rs index 450d564b..c1040ffe 100644 --- a/mls-rs/src/group/proposal_cache.rs +++ b/mls-rs/src/group/proposal_cache.rs @@ -249,7 +249,7 @@ impl GroupState { .await?; #[cfg(feature = "by_ref_proposal")] - let rejected_proposals = rejected_proposals( + let unused_proposals = unused_proposals( match direction { CommitDirection::Send => all_proposals, CommitDirection::Receive => self.proposals.proposals.iter().collect(), @@ -274,7 +274,7 @@ impl GroupState { external_init_index: applier_output.external_init_index, indexes_of_added_kpkgs: applier_output.indexes_of_added_kpkgs, #[cfg(feature = "by_ref_proposal")] - rejected_proposals, + unused_proposals, }) } } @@ -297,7 +297,7 @@ fn has_ref(proposals: &ProposalBundle, reference: &ProposalRef) -> bool { } #[cfg(feature = "by_ref_proposal")] -fn rejected_proposals( +fn unused_proposals( all_proposals: ProposalBundle, accepted_proposals: &ProposalBundle, ) -> Vec> { @@ -802,7 +802,7 @@ mod tests { external_init_index: None, indexes_of_added_kpkgs: vec![LeafIndex(1)], #[cfg(feature = "state_update")] - rejected_proposals: vec![], + unused_proposals: vec![], applied_proposals: bundle, }; @@ -903,7 +903,7 @@ mod tests { assert_eq!(expected_state.public_tree, state.public_tree); #[cfg(feature = "state_update")] - assert_eq!(expected_state.rejected_proposals, state.rejected_proposals); + assert_eq!(expected_state.unused_proposals, state.unused_proposals); } #[maybe_async::test(not(mls_build_async), async(mls_build_async, crate::futures_test))] @@ -2036,10 +2036,7 @@ mod tests { assert_eq!(processed_proposals.0, Vec::new()); #[cfg(feature = "state_update")] - assert_eq!( - processed_proposals.1.rejected_proposals, - vec![proposal_info] - ); + assert_eq!(processed_proposals.1.unused_proposals, vec![proposal_info]); } #[maybe_async::test(not(mls_build_async), async(mls_build_async, crate::futures_test))] @@ -2086,10 +2083,7 @@ mod tests { assert_eq!(processed_proposals.0, Vec::new()); #[cfg(feature = "state_update")] - assert_eq!( - processed_proposals.1.rejected_proposals, - vec![proposal_info] - ); + assert_eq!(processed_proposals.1.unused_proposals, vec![proposal_info]); } #[maybe_async::test(not(mls_build_async), async(mls_build_async, crate::futures_test))] @@ -2136,13 +2130,8 @@ mod tests { assert_eq!(processed_proposals.0, Vec::new()); - // Alice didn't propose the update. Bob did. That's why it is not returned in the list of - // rejected proposals. #[cfg(feature = "state_update")] - assert_eq!( - processed_proposals.1.rejected_proposals, - vec![proposal_info] - ); + assert_eq!(processed_proposals.1.unused_proposals, vec![proposal_info]); } #[maybe_async::test(not(mls_build_async), async(mls_build_async, crate::futures_test))] @@ -2201,10 +2190,7 @@ mod tests { assert_eq!(processed_proposals.0, Vec::new()); #[cfg(feature = "state_update")] - assert_eq!( - processed_proposals.1.rejected_proposals, - vec![proposal_info] - ); + assert_eq!(processed_proposals.1.unused_proposals, vec![proposal_info]); } #[cfg(feature = "psk")] @@ -2286,10 +2272,7 @@ mod tests { assert_eq!(processed_proposals.0, Vec::new()); #[cfg(feature = "state_update")] - assert_eq!( - processed_proposals.1.rejected_proposals, - vec![proposal_info] - ); + assert_eq!(processed_proposals.1.unused_proposals, vec![proposal_info]); } #[cfg(feature = "psk")] @@ -2358,10 +2341,7 @@ mod tests { assert_eq!(processed_proposals.0, Vec::new()); #[cfg(feature = "state_update")] - assert_eq!( - processed_proposals.1.rejected_proposals, - vec![proposal_info] - ); + assert_eq!(processed_proposals.1.unused_proposals, vec![proposal_info]); } #[cfg(feature = "psk")] @@ -2460,10 +2440,7 @@ mod tests { assert_eq!(processed_proposals.0, Vec::new()); #[cfg(feature = "state_update")] - assert_eq!( - processed_proposals.1.rejected_proposals, - vec![proposal_info] - ); + assert_eq!(processed_proposals.1.unused_proposals, vec![proposal_info]); } #[maybe_async::test(not(mls_build_async), async(mls_build_async, crate::futures_test))] @@ -2517,10 +2494,7 @@ mod tests { assert_eq!(processed_proposals.0, Vec::new()); #[cfg(feature = "state_update")] - assert_eq!( - processed_proposals.1.rejected_proposals, - vec![proposal_info] - ); + assert_eq!(processed_proposals.1.unused_proposals, vec![proposal_info]); } #[maybe_async::test(not(mls_build_async), async(mls_build_async, crate::futures_test))] @@ -2571,10 +2545,7 @@ mod tests { assert_eq!(processed_proposals.0, Vec::new()); #[cfg(feature = "state_update")] - assert_eq!( - processed_proposals.1.rejected_proposals, - vec![proposal_info] - ); + assert_eq!(processed_proposals.1.unused_proposals, vec![proposal_info]); } #[maybe_async::test(not(mls_build_async), async(mls_build_async, crate::futures_test))] @@ -2628,7 +2599,7 @@ mod tests { assert_eq!(processed_proposals.0, vec![remove_ref.into()]); #[cfg(feature = "state_update")] - assert_eq!(processed_proposals.1.rejected_proposals, vec![update_info]); + assert_eq!(processed_proposals.1.unused_proposals, vec![update_info]); } #[cfg_attr(not(mls_build_async), maybe_async::must_be_sync)] @@ -2699,7 +2670,7 @@ mod tests { #[cfg(feature = "state_update")] assert_matches!( - &*processed_proposals.1.rejected_proposals, + &*processed_proposals.1.unused_proposals, [rejected_add_info] if committed_add_ref != rejected_add_info.proposal_ref().unwrap() && add_refs.contains(rejected_add_info.proposal_ref().unwrap()) ); } @@ -2745,7 +2716,7 @@ mod tests { // Bob proposed the update, so it is not listed as rejected when Alice commits it because // she didn't propose it. #[cfg(feature = "state_update")] - assert_eq!(processed_proposals.1.rejected_proposals, vec![update_info]); + assert_eq!(processed_proposals.1.unused_proposals, vec![update_info]); } #[maybe_async::test(not(mls_build_async), async(mls_build_async, crate::futures_test))] @@ -2836,10 +2807,7 @@ mod tests { assert_eq!(processed_proposals.0, Vec::new()); #[cfg(feature = "state_update")] - assert_eq!( - processed_proposals.1.rejected_proposals, - vec![proposal_info] - ); + assert_eq!(processed_proposals.1.unused_proposals, vec![proposal_info]); } #[cfg(feature = "psk")] @@ -2917,18 +2885,15 @@ mod tests { assert!(proposal_info.contains(&committed_info)); - // The list of rejected proposals may be empty if Bob's proposal was the one that got - // rejected. #[cfg(feature = "state_update")] - match &*processed_proposals.1.rejected_proposals { + match &*processed_proposals.1.unused_proposals { [r] => { assert_ne!(*r, committed_info); assert!(proposal_info.contains(r)); } - [] => {} _ => panic!( - "Expected zero or one proposal reference in {:?}", - processed_proposals.1.rejected_proposals + "Expected one proposal reference in {:?}", + processed_proposals.1.unused_proposals ), } } @@ -3053,7 +3018,7 @@ mod tests { #[cfg(feature = "state_update")] assert_matches!( - &*processed_proposals.1.rejected_proposals, + &*processed_proposals.1.unused_proposals, [rejected_gce_info] if committed_gce_info != *rejected_gce_info && gce_info.contains(rejected_gce_info) ); } @@ -3131,10 +3096,7 @@ mod tests { assert_eq!(processed_proposals.0, Vec::new()); #[cfg(feature = "state_update")] - assert_eq!( - processed_proposals.1.rejected_proposals, - vec![proposal_info] - ); + assert_eq!(processed_proposals.1.unused_proposals, vec![proposal_info]); } #[maybe_async::test(not(mls_build_async), async(mls_build_async, crate::futures_test))] @@ -3194,7 +3156,7 @@ mod tests { assert_eq!(processed_proposals.0, vec![add_ref.into()]); #[cfg(feature = "state_update")] - assert_eq!(processed_proposals.1.rejected_proposals, vec![reinit_info]); + assert_eq!(processed_proposals.1.unused_proposals, vec![reinit_info]); } #[maybe_async::test(not(mls_build_async), async(mls_build_async, crate::futures_test))] @@ -3259,15 +3221,14 @@ mod tests { #[cfg(feature = "state_update")] { - let (rejected_ref, rejected_proposal) = match &*processed_proposals.1.rejected_proposals - { + let (rejected_ref, unused_proposal) = match &*processed_proposals.1.unused_proposals { [r] => (r.proposal_ref().unwrap().clone(), r.proposal.clone()), p => panic!("Expected single proposal but found {p:?}"), }; assert_ne!(rejected_ref, *processed_ref); assert!(rejected_ref == reinit_ref || rejected_ref == other_reinit_ref); - assert!(rejected_proposal == reinit || rejected_proposal == other_reinit); + assert!(unused_proposal == reinit || unused_proposal == other_reinit); } } @@ -3326,7 +3287,7 @@ mod tests { #[cfg(feature = "state_update")] assert_eq!( - processed_proposals.1.rejected_proposals, + processed_proposals.1.unused_proposals, vec![external_init_info] ); } @@ -3397,10 +3358,7 @@ mod tests { assert_eq!(processed_proposals.0, Vec::new()); #[cfg(feature = "state_update")] - assert_eq!( - processed_proposals.1.rejected_proposals, - vec![proposal_info] - ); + assert_eq!(processed_proposals.1.unused_proposals, vec![proposal_info]); } #[maybe_async::test(not(mls_build_async), async(mls_build_async, crate::futures_test))] @@ -3646,7 +3604,7 @@ mod tests { assert_eq!(processed_proposals.0, Vec::new()); #[cfg(feature = "state_update")] - assert_eq!(processed_proposals.1.rejected_proposals, vec![add_info]); + assert_eq!(processed_proposals.1.unused_proposals, vec![add_info]); } #[cfg(feature = "custom_proposal")] @@ -3692,7 +3650,7 @@ mod tests { assert_eq!(processed_proposals.0, Vec::new()); #[cfg(feature = "state_update")] - assert_eq!(processed_proposals.1.rejected_proposals, vec![custom_info]); + assert_eq!(processed_proposals.1.unused_proposals, vec![custom_info]); } #[cfg(feature = "custom_proposal")] @@ -3776,10 +3734,7 @@ mod tests { assert_eq!(processed_proposals.0, Vec::new()); #[cfg(feature = "state_update")] - assert_eq!( - processed_proposals.1.rejected_proposals, - vec![proposal_info] - ); + assert_eq!(processed_proposals.1.unused_proposals, vec![proposal_info]); } #[cfg(feature = "psk")] @@ -3851,10 +3806,7 @@ mod tests { assert_eq!(processed_proposals.0, Vec::new()); #[cfg(feature = "state_update")] - assert_eq!( - processed_proposals.1.rejected_proposals, - vec![proposal_info] - ); + assert_eq!(processed_proposals.1.unused_proposals, vec![proposal_info]); } #[maybe_async::test(not(mls_build_async), async(mls_build_async, crate::futures_test))] @@ -4199,10 +4151,10 @@ mod tests { .await .unwrap(); - let [p] = &state.rejected_proposals[..] else { + let [p] = &state.unused_proposals[..] else { panic!( - "Expected single rejected proposal but got {:?}", - state.rejected_proposals + "Expected single unused proposal but got {:?}", + state.unused_proposals ); }; diff --git a/mls-rs/src/tree_kem/update_path.rs b/mls-rs/src/tree_kem/update_path.rs index 5e975b83..b3113d06 100644 --- a/mls-rs/src/tree_kem/update_path.rs +++ b/mls-rs/src/tree_kem/update_path.rs @@ -180,7 +180,7 @@ mod tests { indexes_of_added_kpkgs: vec![], external_init_index: None, #[cfg(feature = "state_update")] - rejected_proposals: vec![], + unused_proposals: vec![], } }