diff --git a/xmtp_mls/src/groups/group_mutable_metadata.rs b/xmtp_mls/src/groups/group_mutable_metadata.rs index f1c2ba58c..8f6eae209 100644 --- a/xmtp_mls/src/groups/group_mutable_metadata.rs +++ b/xmtp_mls/src/groups/group_mutable_metadata.rs @@ -17,6 +17,8 @@ pub enum GroupMutableMetadataError { Deserialization(#[from] prost::DecodeError), #[error("missing extension")] MissingExtension, + #[error("mutable extension updates only")] + NonMutableExtensionUpdate, } #[derive(Debug, Clone, PartialEq)] diff --git a/xmtp_mls/src/groups/group_permissions.rs b/xmtp_mls/src/groups/group_permissions.rs index 872dac27c..11eb7b1ac 100644 --- a/xmtp_mls/src/groups/group_permissions.rs +++ b/xmtp_mls/src/groups/group_permissions.rs @@ -14,14 +14,12 @@ use xmtp_proto::xmtp::mls::message_contents::{ PolicySet as PolicySetProto, }; -use super::validated_commit::{ - AggregatedMembershipChange, CommitParticipant, MetadataUpdate, ValidatedCommit, -}; +use super::validated_commit::{AggregatedMembershipChange, CommitParticipant, ValidatedCommit}; // A trait for policies that can update Metadata for the group pub trait MetadataPolicy: std::fmt::Debug { - fn evaluate(&self, actor: &CommitParticipant, change: &MetadataUpdate) -> bool; + fn evaluate(&self, actor: &CommitParticipant, change: bool) -> bool; fn to_proto(&self) -> Result; } @@ -35,14 +33,11 @@ pub enum MetadataBasePolicies { } impl MetadataPolicy for MetadataBasePolicies { - fn evaluate(&self, actor: &CommitParticipant, change: &MetadataUpdate) -> bool { - if change.updates_outside_mutable_metadata { - return false; - } + fn evaluate(&self, actor: &CommitParticipant, change: bool) -> bool { match self { MetadataBasePolicies::Allow => true, - MetadataBasePolicies::Deny => !change.field_updated, - MetadataBasePolicies::AllowIfActorCreator => actor.is_creator || !change.field_updated, + MetadataBasePolicies::Deny => !change, + MetadataBasePolicies::AllowIfActorCreator => actor.is_creator || !change, } } @@ -134,7 +129,7 @@ impl TryFrom for MetadataPolicies { } impl MetadataPolicy for MetadataPolicies { - fn evaluate(&self, actor: &CommitParticipant, change: &MetadataUpdate) -> bool { + fn evaluate(&self, actor: &CommitParticipant, change: bool) -> bool { match self { MetadataPolicies::Standard(policy) => policy.evaluate(actor, change), MetadataPolicies::AndCondition(policy) => policy.evaluate(actor, change), @@ -164,7 +159,7 @@ impl MetadataAndCondition { } impl MetadataPolicy for MetadataAndCondition { - fn evaluate(&self, actor: &CommitParticipant, change: &MetadataUpdate) -> bool { + fn evaluate(&self, actor: &CommitParticipant, change: bool) -> bool { self.policies .iter() .all(|policy| policy.evaluate(actor, change)) @@ -199,7 +194,7 @@ impl MetadataAnyCondition { } impl MetadataPolicy for MetadataAnyCondition { - fn evaluate(&self, actor: &CommitParticipant, change: &MetadataUpdate) -> bool { + fn evaluate(&self, actor: &CommitParticipant, change: bool) -> bool { self.policies .iter() .any(|policy| policy.evaluate(actor, change)) @@ -477,7 +472,7 @@ impl PolicySet { &self.remove_installation_policy, &commit.actor, ) & self.evaluate_metadata_policy( - &commit.group_name_updated, + commit.group_name_updated, &self.update_group_name_policy, &commit.actor, ) @@ -509,7 +504,7 @@ impl PolicySet { fn evaluate_metadata_policy

( &self, - change: &MetadataUpdate, + change: bool, policy: &P, actor: &CommitParticipant, ) -> bool diff --git a/xmtp_mls/src/groups/validated_commit.rs b/xmtp_mls/src/groups/validated_commit.rs index 2b0e5472b..31847b7d1 100644 --- a/xmtp_mls/src/groups/validated_commit.rs +++ b/xmtp_mls/src/groups/validated_commit.rs @@ -79,16 +79,6 @@ pub struct AggregatedMembershipChange { pub(crate) is_creator: bool, } -// Account information for Metadata Update used for validation -#[derive(Clone, Debug)] -pub struct MetadataUpdate { - #[allow(dead_code)] - pub(crate) updates_outside_mutable_metadata: bool, - #[allow(dead_code)] - pub(crate) field_updated: bool, - // TODO: other metadata fields will go here -} - // A parsed and validated commit that we can apply permissions and rules to #[derive(Clone, Debug)] pub struct ValidatedCommit { @@ -97,7 +87,7 @@ pub struct ValidatedCommit { pub(crate) members_removed: Vec, pub(crate) installations_added: Vec, pub(crate) installations_removed: Vec, - pub(crate) group_name_updated: MetadataUpdate, + pub(crate) group_name_updated: bool, } impl ValidatedCommit { @@ -147,6 +137,9 @@ impl ValidatedCommit { &group_metadata, )?; + // We don't allow commits that update Group Context Extensions outside type Unknown(MUTABLE_METADATA_EXTENSION_ID) + ensure_extensions_valid(staged_commit, openmls_group)?; + let group_name_updated = get_group_name_updated(staged_commit, openmls_group)?; let validated_commit = Self { @@ -363,17 +356,13 @@ fn get_removed_members( fn get_group_name_updated( staged_commit: &StagedCommit, openmls_group: &OpenMlsGroup, -) -> Result { +) -> Result { let existing_mutable_metadata = extract_group_mutable_metadata(openmls_group)?; - // TODO verify we have not updated extensions outside MUTABLE_METADATA_EXTENSION_ID - let updates_outside_mutable_metadata = false; - // Iterate through each proposal for proposal in staged_commit.queued_proposals() { if let Proposal::GroupContextExtensions(extension_proposal) = proposal.proposal() { let extensions = extension_proposal.extensions(); - - // Check each extension to see if it updates metadata group name + // Check each MUTABLE_METADATA extension to see if it updates metadata group name for extension in extensions.iter() { if let Extension::Unknown(MUTABLE_METADATA_EXTENSION_ID, UnknownExtension(data)) = extension @@ -381,10 +370,7 @@ fn get_group_name_updated( match GroupMutableMetadata::try_from(data) { Ok(metadata) => { if metadata.group_name != existing_mutable_metadata.group_name { - return Ok(MetadataUpdate { - updates_outside_mutable_metadata, - field_updated: true, - }); + return Ok(true); } } Err(e) => return Err(CommitValidationError::from(e)), @@ -393,10 +379,32 @@ fn get_group_name_updated( } } } - Ok(MetadataUpdate { - updates_outside_mutable_metadata, - field_updated: false, - }) + Ok(false) +} + +fn ensure_extensions_valid( + staged_commit: &StagedCommit, + openmls_group: &OpenMlsGroup, +) -> Result<(), CommitValidationError> { + let mut existing_extensions = openmls_group.export_group_context().extensions().clone(); + existing_extensions.remove(openmls::extensions::ExtensionType::Unknown( + MUTABLE_METADATA_EXTENSION_ID, + )); + // Iterate through each proposal + for proposal in staged_commit.queued_proposals() { + if let Proposal::GroupContextExtensions(extension_proposal) = proposal.proposal() { + let mut extensions = extension_proposal.extensions().clone(); + extensions.remove(openmls::extensions::ExtensionType::Unknown( + MUTABLE_METADATA_EXTENSION_ID, + )); + if extensions != existing_extensions { + return Err(CommitValidationError::GroupMutableMetadata( + GroupMutableMetadataError::NonMutableExtensionUpdate, + )); + } + } + } + Ok(()) } impl From for GroupMembershipChanges {