Cherry picks bug fix for past epochs

openmls/src/group/core_group/process.rs

@@ -3,7 +3,7 @@ use core_group::proposals::QueuedProposal;
 use crate::{
     framing::mls_content::FramedContentBody,
     group::{
-        errors::{MergeCommitError, StageCommitError, ValidationError},
+        errors::{StageCommitError, ValidationError},
         mls_group::errors::ProcessMessageError,
     },
 };
@@ -288,26 +288,4 @@ impl CoreGroup {
 
         Ok((old_epoch_keypairs, leaf_node_keypairs))
     }
-
-    /// Merge a [StagedCommit] into the group after inspection
-    pub(crate) fn merge_staged_commit<Provider: OpenMlsProvider>(
-        &mut self,
-        provider: &Provider,
-        staged_commit: StagedCommit,
-        proposal_store: &mut ProposalStore,
-    ) -> Result<(), MergeCommitError<Provider::StorageError>> {
-        // Save the past epoch
-        let past_epoch = self.context().epoch();
-        // Get all the full leaves
-        let leaves = self.public_group().members().collect();
-        // Merge the staged commit into the group state and store the secret tree from the
-        // previous epoch in the message secrets store.
-        if let Some(message_secrets) = self.merge_commit(provider, staged_commit)? {
-            self.message_secrets_store
-                .add(past_epoch, message_secrets, leaves);
-        }
-        // Empty the proposal store
-        proposal_store.empty();
-        Ok(())
-    }
 }

openmls/src/group/core_group/staged_commit.rs

@@ -7,7 +7,8 @@ use self::public_group::staged_commit::PublicStagedCommitState;
 
 use super::{super::errors::*, proposals::ProposalStore, *};
 use crate::{
-    ciphersuite::Secret, framing::mls_auth_content::AuthenticatedContent,
+    ciphersuite::hash_ref::ProposalRef, ciphersuite::Secret,
+    framing::mls_auth_content::AuthenticatedContent,
     treesync::node::encryption_keys::EncryptionKeyPair,
 };
 
@@ -318,7 +319,7 @@ impl CoreGroup {
         &mut self,
         provider: &Provider,
         staged_commit: StagedCommit,
-    ) -> Result<Option<MessageSecrets>, MergeCommitError<Provider::StorageError>> {
+    ) -> Result<(), MergeCommitError<Provider::StorageError>> {
         // Get all keypairs from the old epoch, so we can later store the ones
         // that are still relevant in the new epoch.
         let old_epoch_keypairs = self.read_epoch_keypairs(provider.storage());
@@ -328,9 +329,15 @@ impl CoreGroup {
                     .merge_diff(staged_state.into_staged_diff());
                 self.store(provider.storage())
                     .map_err(MergeCommitError::StorageError)?;
-                Ok(None)
+                Ok(())
             }
             StagedCommitState::GroupMember(state) => {
+                // Save the past epoch
+                let past_epoch = self.context().epoch();
+                // Get all the full leaves
+                let leaves = self.public_group().members().collect();
+                // Merge the staged commit into the group state and store the secret tree from the
+                // previous epoch in the message secrets store.
                 self.group_epoch_secrets = state.group_epoch_secrets;
 
                 // Replace the previous message secrets with the new ones and return the previous message secrets
@@ -339,6 +346,8 @@ impl CoreGroup {
                     &mut message_secrets,
                     self.message_secrets_store.message_secrets_mut(),
                 );
+                self.message_secrets_store
+                    .add(past_epoch, message_secrets, leaves);
 
                 self.public_group.merge_diff(state.staged_diff);
 
@@ -400,7 +409,13 @@ impl CoreGroup {
                         .map_err(MergeCommitError::StorageError)?;
                 }
 
-                Ok(Some(message_secrets))
+                // Empty the proposal store
+                storage
+                    .clear_proposal_queue::<GroupId, ProposalRef>(group_id)
+                    .map_err(MergeCommitError::StorageError)?;
+                self.proposal_store_mut().empty();
+
+                Ok(())
             }
         }
     }

openmls/src/group/mls_group/processing.rs

@@ -143,8 +143,7 @@ impl MlsGroup {
             .map_err(MergeCommitError::StorageError)?;
 
         // Merge staged commit
-        self.group
-            .merge_staged_commit(provider, staged_commit, &mut self.proposal_store)?;
+        self.group.merge_commit(provider, staged_commit)?;
 
         // Extract and store the resumption psk for the current epoch
         let resumption_psk = self.group.group_epoch_secrets().resumption_psk();

openmls/src/group/tests/test_past_secrets.rs

@@ -1,16 +1,22 @@
 //! This module contains tests regarding the use of [`MessageSecretsStore`] in [`MlsGroup`]
 
-use super::utils::{generate_credential_with_key, generate_key_package};
+use crate::group::tests_and_kats::utils::{generate_credential_with_key, generate_key_package};
 use crate::{
     framing::{MessageDecryptionError, MlsMessageIn, ProcessedMessageContent},
     group::*,
+    treesync::LeafNodeParameters,
 };
 
+use openmls_traits::OpenMlsProvider as _;
+
 #[openmls_test::openmls_test]
-fn test_past_secrets_in_group(
+fn test_past_secrets_in_group<Provider: crate::storage::OpenMlsProvider>(
     ciphersuite: Ciphersuite,
-    provider: &impl crate::storage::OpenMlsProvider,
+    provider: &Provider,
 ) {
+    let alice_provider = &mut Provider::default();
+    let bob_provider = &mut Provider::default();
+
     // Test this for different parameters
     for max_epochs in (0..10usize).step_by(2) {
         let group_id = GroupId::from_slice(b"Test Group");
@@ -19,19 +25,19 @@ fn test_past_secrets_in_group(
         let alice_credential_with_keys = generate_credential_with_key(
             b"Alice".to_vec(),
             ciphersuite.signature_algorithm(),
-            provider,
+            alice_provider,
         );
         let bob_credential_with_keys = generate_credential_with_key(
             b"Bob".to_vec(),
             ciphersuite.signature_algorithm(),
-            provider,
+            bob_provider,
         );
 
         // Generate KeyPackages
         let bob_key_package = generate_key_package(
             ciphersuite,
             Extensions::empty(),
-            provider,
+            bob_provider,
             bob_credential_with_keys,
         );
 
@@ -44,7 +50,7 @@ fn test_past_secrets_in_group(
 
         // === Alice creates a group ===
         let mut alice_group = MlsGroup::new_with_group_id(
-            provider,
+            alice_provider,
             &alice_credential_with_keys.signer,
             &mls_group_create_config,
             group_id.clone(),
@@ -55,14 +61,14 @@ fn test_past_secrets_in_group(
         // Alice adds Bob
         let (_message, welcome, _group_info) = alice_group
             .add_members(
-                provider,
+                alice_provider,
                 &alice_credential_with_keys.signer,
                 &[bob_key_package.key_package().clone()],
             )
             .expect("An unexpected error occurred.");
 
         alice_group
-            .merge_pending_commit(provider)
+            .merge_pending_commit(alice_provider)
             .expect("error merging pending commit");
 
         let welcome: MlsMessageIn = welcome.into();
@@ -71,13 +77,13 @@ fn test_past_secrets_in_group(
             .expect("expected message to be a welcome");
 
         let mut bob_group = StagedWelcome::new_from_welcome(
-            provider,
+            bob_provider,
             mls_group_create_config.join_config(),
             welcome,
             Some(alice_group.export_ratchet_tree().into()),
         )
         .expect("Error creating staged join from Welcome")
-        .into_group(provider)
+        .into_group(bob_provider)
         .expect("Error creating group from staged join");
 
         // Generate application message for different epochs
@@ -87,34 +93,38 @@ fn test_past_secrets_in_group(
 
         for _ in 0..max_epochs {
             let application_message = alice_group
-                .create_message(provider, &alice_credential_with_keys.signer, &[1, 2, 3])
+                .create_message(
+                    alice_provider,
+                    &alice_credential_with_keys.signer,
+                    &[1, 2, 3],
+                )
                 .expect("An unexpected error occurred.");
 
             application_messages.push(application_message.into_protocol_message().unwrap());
 
             let (message, _welcome, _group_info) = alice_group
-                .self_update(provider, &alice_credential_with_keys.signer)
+                .self_update(alice_provider, &alice_credential_with_keys.signer)
                 .expect("An unexpected error occurred.");
 
             update_commits.push(message.clone());
 
             alice_group
-                .merge_pending_commit(provider)
+                .merge_pending_commit(alice_provider)
                 .expect("error merging pending commit");
         }
 
         // Bob processes all update commits
 
         for update_commit in update_commits {
             let bob_processed_message = bob_group
-                .process_message(provider, update_commit.into_protocol_message().unwrap())
+                .process_message(bob_provider, update_commit.into_protocol_message().unwrap())
                 .expect("An unexpected error occurred.");
 
             if let ProcessedMessageContent::StagedCommitMessage(staged_commit) =
                 bob_processed_message.into_content()
             {
                 bob_group
-                    .merge_staged_commit(provider, *staged_commit)
+                    .merge_staged_commit(bob_provider, *staged_commit)
                     .expect("Error merging commit.");
             } else {
                 unreachable!("Expected a StagedCommit.");
@@ -123,10 +133,14 @@ fn test_past_secrets_in_group(
 
         // === Test application messages from older epochs ===
 
+        let mut bob_group = MlsGroup::load(bob_provider.storage(), &group_id)
+            .expect("error re-loading bob's group")
+            .expect("no such group");
+
         // The first messages should fail
         for application_message in application_messages.iter().take(max_epochs / 2) {
             let err = bob_group
-                .process_message(provider, application_message.clone())
+                .process_message(bob_provider, application_message.clone())
                 .expect_err("An unexpected error occurred.");
             assert!(matches!(
                 err,
@@ -139,7 +153,7 @@ fn test_past_secrets_in_group(
         // The last messages should not fail
         for application_message in application_messages.iter().skip(max_epochs / 2) {
             let bob_processed_message = bob_group
-                .process_message(provider, application_message.clone())
+                .process_message(bob_provider, application_message.clone())
                 .expect("An unexpected error occurred.");
 
             if let ProcessedMessageContent::ApplicationMessage(application_message) =