From 6cf26ca4d5be92c4daf2d4ba2a817393ab3e6b97 Mon Sep 17 00:00:00 2001 From: cameronvoell Date: Mon, 1 Jul 2024 11:13:02 -0700 Subject: [PATCH] configure metadata update evaluation for fields with unknown policies --- xmtp_mls/src/configuration.rs | 4 + xmtp_mls/src/groups/group_permissions.rs | 137 +++++++++++++++++++---- 2 files changed, 118 insertions(+), 23 deletions(-) diff --git a/xmtp_mls/src/configuration.rs b/xmtp_mls/src/configuration.rs index 70e0a48c2..31671db79 100644 --- a/xmtp_mls/src/configuration.rs +++ b/xmtp_mls/src/configuration.rs @@ -44,3 +44,7 @@ pub const GROUP_PERMISSIONS_EXTENSION_ID: u16 = 0xff02; pub const DEFAULT_GROUP_NAME: &str = ""; pub const DEFAULT_GROUP_DESCRIPTION: &str = ""; pub const DEFAULT_GROUP_IMAGE_URL_SQUARE: &str = ""; + +// If a metadata field name starts with this character, +// and it does not have a policy set, it is a super admin only field +pub const SUPER_ADMIN_METADATA_PREFIX: &str = "_"; diff --git a/xmtp_mls/src/groups/group_permissions.rs b/xmtp_mls/src/groups/group_permissions.rs index 790be5f0c..90df943ff 100644 --- a/xmtp_mls/src/groups/group_permissions.rs +++ b/xmtp_mls/src/groups/group_permissions.rs @@ -25,7 +25,7 @@ use xmtp_proto::xmtp::mls::message_contents::{ PermissionsUpdatePolicy as PermissionsPolicyProto, PolicySet as PolicySetProto, }; -use crate::configuration::GROUP_PERMISSIONS_EXTENSION_ID; +use crate::configuration::{GROUP_PERMISSIONS_EXTENSION_ID, SUPER_ADMIN_METADATA_PREFIX}; use super::{ group_mutable_metadata::GroupMutableMetadata, @@ -923,8 +923,7 @@ impl PolicySet { { changes.all(|change| { if let Some(policy) = policies.get(&change.field_name) { - let is_ok = policy.evaluate(actor, change); - if !is_ok { + if !policy.evaluate(actor, change) { log::info!( "Policy for field {} failed for actor {:?} and change {:?}", change.field_name, @@ -933,13 +932,25 @@ impl PolicySet { ); return false; } - return is_ok; + return true; } - log::info!( - "Missing policy for changed metadata field: {}", - change.field_name - ); - false + // Policy is not found for metadata change, let's check if the new field contains the super_admin prefix + // and evaluate accordingly + let policy_for_unrecognized_field = + if change.field_name.starts_with(SUPER_ADMIN_METADATA_PREFIX) { + MetadataPolicies::allow_if_actor_super_admin() + } else { + // Otherwise we default to admin only for fields with missing policies + MetadataPolicies::allow_if_actor_admin() + }; + if !policy_for_unrecognized_field.evaluate(actor, change) { + log::info!( + "Metadata field update with unknown policy was denied: {}", + change.field_name + ); + return false; + } + true }) } @@ -1161,14 +1172,15 @@ mod tests { member_removed: Option, metadata_fields_changed: Option>, permissions_changed: bool, + actor_is_admin: bool, actor_is_super_admin: bool, ) -> ValidatedCommit { - let actor = build_actor(None, None, actor_is_super_admin, actor_is_super_admin); + let actor = build_actor(None, None, actor_is_admin, actor_is_super_admin); let build_membership_change = |same_address_as_actor| { if same_address_as_actor { vec![build_change( Some(actor.inbox_id.clone()), - actor_is_super_admin, + actor_is_admin, actor_is_super_admin, )] } else { @@ -1209,7 +1221,7 @@ mod tests { PermissionsPolicies::allow_if_actor_super_admin(), ); - let commit = build_validated_commit(Some(true), Some(true), None, false, false); + let commit = build_validated_commit(Some(true), Some(true), None, false, false, false); assert!(permissions.evaluate_commit(&commit)); } @@ -1224,10 +1236,12 @@ mod tests { PermissionsPolicies::allow_if_actor_super_admin(), ); - let member_added_commit = build_validated_commit(Some(false), None, None, false, false); + let member_added_commit = + build_validated_commit(Some(false), None, None, false, false, false); assert!(!permissions.evaluate_commit(&member_added_commit)); - let member_removed_commit = build_validated_commit(None, Some(false), None, false, false); + let member_removed_commit = + build_validated_commit(None, Some(false), None, false, false, false); assert!(!permissions.evaluate_commit(&member_removed_commit)); } @@ -1243,15 +1257,16 @@ mod tests { ); // Can not remove the creator if they are the only super admin - let commit_with_creator = build_validated_commit(Some(true), Some(true), None, false, true); + let commit_with_creator = + build_validated_commit(Some(true), Some(true), None, false, false, true); assert!(!permissions.evaluate_commit(&commit_with_creator)); let commit_with_creator = - build_validated_commit(Some(true), Some(false), None, false, true); + build_validated_commit(Some(true), Some(false), None, false, false, true); assert!(permissions.evaluate_commit(&commit_with_creator)); let commit_without_creator = - build_validated_commit(Some(true), Some(true), None, false, false); + build_validated_commit(Some(true), Some(true), None, false, false, false); assert!(!permissions.evaluate_commit(&commit_without_creator)); } @@ -1266,11 +1281,12 @@ mod tests { PermissionsPolicies::allow_if_actor_super_admin(), ); - let commit_with_same_member = build_validated_commit(Some(true), None, None, false, false); + let commit_with_same_member = + build_validated_commit(Some(true), None, None, false, false, false); assert!(permissions.evaluate_commit(&commit_with_same_member)); let commit_with_different_member = - build_validated_commit(Some(false), None, None, false, false); + build_validated_commit(Some(false), None, None, false, false, false); assert!(!permissions.evaluate_commit(&commit_with_different_member)); } @@ -1288,7 +1304,8 @@ mod tests { PermissionsPolicies::allow_if_actor_super_admin(), ); - let member_added_commit = build_validated_commit(Some(true), None, None, false, false); + let member_added_commit = + build_validated_commit(Some(true), None, None, false, false, false); assert!(!permissions.evaluate_commit(&member_added_commit)); } @@ -1306,7 +1323,8 @@ mod tests { PermissionsPolicies::allow_if_actor_super_admin(), ); - let member_added_commit = build_validated_commit(Some(true), None, None, false, false); + let member_added_commit = + build_validated_commit(Some(true), None, None, false, false, false); assert!(permissions.evaluate_commit(&member_added_commit)); } @@ -1354,6 +1372,7 @@ mod tests { Some(vec![MetadataField::GroupName.to_string()]), false, false, + false, ); assert!(allow_permissions.evaluate_commit(&member_added_commit)); @@ -1450,11 +1469,83 @@ mod tests { ); // Commit should fail because actor is not superadmin - let commit = build_validated_commit(None, None, None, true, false); + let commit = build_validated_commit(None, None, None, true, false, false); assert!(!permissions.evaluate_commit(&commit)); // Commit should pass because actor is superadmin - let commit = build_validated_commit(None, None, None, true, true); + let commit = build_validated_commit(None, None, None, true, false, true); assert!(permissions.evaluate_commit(&commit)); } + + #[test] + fn test_evaluate_field_with_unknown_policy() { + // Create a group whose default metadata can be updated by any member + let permissions = PolicySet::new( + MembershipPolicies::allow(), + MembershipPolicies::allow(), + MetadataPolicies::default_map(MetadataPolicies::allow()), + PermissionsPolicies::allow_if_actor_super_admin(), + PermissionsPolicies::allow_if_actor_super_admin(), + PermissionsPolicies::allow_if_actor_super_admin(), + ); + + // Non admin, non super admin can update group name + let name_updated_commit = build_validated_commit( + None, + None, + Some(vec![MetadataField::GroupName.to_string()]), + false, + false, + false, + ); + assert!(permissions.evaluate_commit(&name_updated_commit)); + + // Non admin, non super admin can NOT update non existing field + let non_existing_field_updated_commit = build_validated_commit( + None, + None, + Some(vec!["non_existing_field".to_string()]), + false, + false, + false, + ); + assert!(!permissions.evaluate_commit(&non_existing_field_updated_commit)); + + // Admin can update non existing field + let non_existing_field_updated_commit = build_validated_commit( + None, + None, + Some(vec!["non_existing_field".to_string()]), + false, + true, + false, + ); + assert!(permissions.evaluate_commit(&non_existing_field_updated_commit)); + + // Admin can NOT update non existing field that starts with super_admin only prefix + let non_existing_field_updated_commit = build_validated_commit( + None, + None, + Some(vec![ + SUPER_ADMIN_METADATA_PREFIX.to_string() + "non_existing_field", + ]), + false, + true, + false, + ); + assert!(!permissions.evaluate_commit(&non_existing_field_updated_commit)); + + // Super Admin CAN update non existing field that starts with super_admin only prefix + let non_existing_field_updated_commit = build_validated_commit( + None, + None, + Some(vec![ + SUPER_ADMIN_METADATA_PREFIX.to_string() + "non_existing_field", + ]), + false, + false, + true, + ); + assert!(permissions.evaluate_commit(&non_existing_field_updated_commit)); + } }