-
Notifications
You must be signed in to change notification settings - Fork 40
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.
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
Remove pinned frames url from metadata #1534
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change looks good. Even in rare cases where a user on an old version tried to update this field on a new group that doesnt have the field, we would not have a fork. 👌 see metadata update eval logic for more details:
libxmtp/xmtp_mls/src/groups/group_permissions.rs
Lines 1023 to 1064 in 3b01e70
/// Evaluates metadata policies for a given set of changes. | |
fn evaluate_metadata_policy<'a, I>( | |
&self, | |
mut changes: I, | |
policies: &HashMap<String, MetadataPolicies>, | |
actor: &CommitParticipant, | |
) -> bool | |
where | |
I: Iterator<Item = &'a MetadataFieldChange>, | |
{ | |
changes.all(|change| { | |
if let Some(policy) = policies.get(&change.field_name) { | |
if !policy.evaluate(actor, change) { | |
tracing::info!( | |
"Policy for field {} failed for actor {:?} and change {:?}", | |
change.field_name, | |
actor, | |
change | |
); | |
return false; | |
} | |
return true; | |
} | |
// 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) { | |
tracing::info!( | |
"Metadata field update with unknown policy was denied: {}", | |
change.field_name | |
); | |
return false; | |
} | |
true | |
}) | |
} |
Talked with product and it doesn't currently feel like there is any value in leaving a url field in the metadata at all so lets just remove it entirely.
THIS IS A BREAKING GROUP CHANGE