ensure gce proposals only update mutable metadata

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)]

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<MetadataPolicyProto, PolicyError>;
 }
 
@@ -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<MetadataPolicyProto> 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<P>(
         &self,
-        change: &MetadataUpdate,
+        change: bool,
         policy: &P,
         actor: &CommitParticipant,
     ) -> bool

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<AggregatedMembershipChange>,
     pub(crate) installations_added: Vec<AggregatedMembershipChange>,
     pub(crate) installations_removed: Vec<AggregatedMembershipChange>,
-    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,28 +356,21 @@ fn get_removed_members(
 fn get_group_name_updated(
     staged_commit: &StagedCommit,
     openmls_group: &OpenMlsGroup,
-) -> Result<MetadataUpdate, CommitValidationError> {
+) -> Result<bool, CommitValidationError> {
     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
                 {
                     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<ValidatedCommit> for GroupMembershipChanges {