Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

configure metadata update evaluation for fields with unknown policies #882

Merged
merged 1 commit into from
Jul 1, 2024
Merged

configure metadata update evaluation for fields with unknown policies #882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions xmtp_mls/src/configuration.rs
View file
Open in desktop
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
Open in desktop
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));
}
}