Skip to content

Commit

Permalink
configure metadata update evaluation for fields with unknown policies (…
Browse files Browse the repository at this point in the history 
…#882)

Co-authored-by: cameronvoell <[email protected]>
  • Loading branch information
@cameronvoell
cameronvoell and cameronvoell authored Jul 1, 2024
1 parent a33bafe commit 7c55ee9
Show file tree
Hide file tree
Showing 2 changed files with 118 additions and 23 deletions.
4 changes: 4 additions & 0 deletions xmtp_mls/src/configuration.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = "_";
137 changes: 114 additions & 23 deletions xmtp_mls/src/groups/group_permissions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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
})
}

Expand Down Expand Up @@ -1161,14 +1172,15 @@ mod tests {
member_removed: Option<bool>,
metadata_fields_changed: Option<Vec<String>>,
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 {
Expand Down Expand Up @@ -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));
}

Expand All @@ -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));
}

Expand All @@ -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));
}

Expand All @@ -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));
}

Expand All @@ -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));
}

Expand All @@ -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));
}

Expand Down Expand Up @@ -1354,6 +1372,7 @@ mod tests {
Some(vec![MetadataField::GroupName.to_string()]),
false,
false,
false,
);

assert!(allow_permissions.evaluate_commit(&member_added_commit));
Expand Down Expand Up @@ -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));
}
}

0 comments on commit 7c55ee9

Please sign in to comment.