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.

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

Mutable Metadata group name Part 1 #643

Merged
merged 6 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 5 additions & 5 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@ futures = "0.3.30"
futures-core = "0.3.30"
hex = "0.4.3"
log = "0.4"
openmls = { git = "https://github.com/xmtp/openmls", rev = "4eee1fc" }
openmls_basic_credential = { git = "https://github.com/xmtp/openmls", rev = "4eee1fc" }
openmls_rust_crypto = { git = "https://github.com/xmtp/openmls", rev = "4eee1fc" }
openmls_traits = { git = "https://github.com/xmtp/openmls", rev = "4eee1fc" }
openmls = { git = "https://github.com/xmtp/openmls", rev = "9288932" }
openmls_basic_credential = { git = "https://github.com/xmtp/openmls", rev = "9288932" }
openmls_rust_crypto = { git = "https://github.com/xmtp/openmls", rev = "9288932" }
openmls_traits = { git = "https://github.com/xmtp/openmls", rev = "9288932" }
prost = "^0.12"
prost-types = "^0.12"
rand = "0.8.5"
Expand Down
10 changes: 5 additions & 5 deletions bindings_ffi/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

78 changes: 78 additions & 0 deletions xmtp_mls/src/groups/group_mutable_metadata.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
use openmls::{
extensions::{Extension, UnknownExtension},
group::MlsGroup as OpenMlsGroup,
};
use prost::Message;
use thiserror::Error;

use xmtp_proto::xmtp::mls::message_contents::GroupMutableMetadataV1 as GroupMutableMetadataProto;

#[derive(Debug, Error)]
pub enum GroupMutableMetadataError {
#[error("serialization: {0}")]
Serialization(#[from] prost::EncodeError),
#[error("deserialization: {0}")]
Deserialization(#[from] prost::DecodeError),
#[error("missing extension")]
MissingExtension,
}

#[derive(Debug, Clone, PartialEq)]
pub struct GroupMutableMetadata {
pub group_name: String,
}

impl GroupMutableMetadata {
pub fn new(group_name: String) -> Self {
Self {
group_name
}
}
}

impl TryFrom<GroupMutableMetadata> for Vec<u8> {
type Error = GroupMutableMetadataError;

fn try_from(value: GroupMutableMetadata) -> Result<Self, Self::Error> {
let mut buf = Vec::new();
let proto_val = GroupMutableMetadataProto {
group_name: value.group_name.clone()
};
proto_val.encode(&mut buf)?;

Ok(buf)
}
}

impl TryFrom<&Vec<u8>> for GroupMutableMetadata {
type Error = GroupMutableMetadataError;

fn try_from(value: &Vec<u8>) -> Result<Self, Self::Error> {
let proto_val = GroupMutableMetadataProto::decode(value.as_slice())?;
Self::from_proto(proto_val)
}
}

impl TryFrom<GroupMutableMetadataProto> for GroupMutableMetadata {
type Error = GroupMutableMetadataError;

fn try_from(value: GroupMutableMetadataProto) -> Result<Self, Self::Error> {
Self::new(value.group_name.clone())
}
}

pub fn extract_group_mutable_metadata(
group: &OpenMlsGroup,
) -> Result<GroupMutableMetadata, GroupMutableMetadataError> {
todo!()
}

#[cfg(test)]
mod tests {

use super::*;
#[test]
fn test_preconfigured_mutable_metadata() {
// TODO add test here
}
}
34 changes: 31 additions & 3 deletions xmtp_mls/src/groups/group_permissions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,19 +231,22 @@ pub struct PolicySet {
pub remove_member_policy: MembershipPolicies,
pub add_installation_policy: MembershipPolicies,
pub remove_installation_policy: MembershipPolicies,
pub update_group_name_policy: MembershipPolicies,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda think we should have a new type of policy for metadata. Some of the MembershipPolicies aren't applicable here (for example, AllowSameMember) and it makes easier to be misused having that as an option.

What if we had a different type, like MetadataPolicy that can be more tailored towards updating metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, ill make that update 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neekolas updating to include a new MetadataPolicy type actually adds a lot of cascading updates, since it touches all the validation logic. I was going to tackle validation logic in part 3, so how do we feel about leaving MetadataPolicy to the part 3 PR?

}

#[allow(dead_code)]
impl PolicySet {
pub fn new(
add_member_policy: MembershipPolicies,
remove_member_policy: MembershipPolicies,
update_group_name_policy: MembershipPolicies,
) -> Self {
Self {
add_member_policy,
remove_member_policy,
add_installation_policy: default_add_installation_policy(),
remove_installation_policy: default_remove_installation_policy(),
update_group_name_policy,
}
}

Expand Down Expand Up @@ -295,6 +298,7 @@ impl PolicySet {
Ok(PolicySetProto {
add_member_policy: Some(self.add_member_policy.to_proto()?),
remove_member_policy: Some(self.remove_member_policy.to_proto()?),
update_group_name_policy: Some(self.update_group_name_policy.to_proto()?),
})
}

Expand All @@ -308,6 +312,11 @@ impl PolicySet {
.remove_member_policy
.ok_or(PolicyError::InvalidPolicy)?,
)?,
MembershipPolicies::try_from(
proto
.update_group_name_policy
.ok_or(PolicyError::InvalidPolicy)?,
)?,
))
}

Expand All @@ -334,14 +343,19 @@ fn default_remove_installation_policy() -> MembershipPolicies {

/// A policy where any member can add or remove any other member
pub(crate) fn policy_everyone_is_admin() -> PolicySet {
PolicySet::new(MembershipPolicies::allow(), MembershipPolicies::allow())
PolicySet::new(
MembershipPolicies::allow(),
MembershipPolicies::allow(),
MembershipPolicies::allow(),
)
}

/// A policy where only the group creator can add or remove members
pub(crate) fn policy_group_creator_is_admin() -> PolicySet {
PolicySet::new(
MembershipPolicies::allow_if_actor_creator(),
MembershipPolicies::allow_if_actor_creator(),
MembershipPolicies::allow_if_actor_creator(),
)
}

Expand Down Expand Up @@ -446,15 +460,23 @@ mod tests {

#[test]
fn test_allow_all() {
let permissions = PolicySet::new(MembershipPolicies::allow(), MembershipPolicies::allow());
let permissions = PolicySet::new(
MembershipPolicies::allow(),
MembershipPolicies::allow(),
MembershipPolicies::allow(),
);

let commit = build_validated_commit(Some(true), Some(true), None, None, false);
assert!(permissions.evaluate_commit(&commit));
}

#[test]
fn test_deny() {
let permissions = PolicySet::new(MembershipPolicies::deny(), MembershipPolicies::deny());
let permissions = PolicySet::new(
MembershipPolicies::deny(),
MembershipPolicies::deny(),
MembershipPolicies::deny(),
);

let member_added_commit = build_validated_commit(Some(false), None, None, None, false);
assert!(!permissions.evaluate_commit(&member_added_commit));
Expand All @@ -478,6 +500,7 @@ mod tests {
let permissions = PolicySet::new(
MembershipPolicies::allow_if_actor_creator(),
MembershipPolicies::allow_if_actor_creator(),
MembershipPolicies::allow_if_actor_creator(),
);

let commit_with_creator = build_validated_commit(Some(true), Some(true), None, None, true);
Expand All @@ -493,6 +516,7 @@ mod tests {
let permissions = PolicySet::new(
MembershipPolicies::allow_same_member(),
MembershipPolicies::deny(),
MembershipPolicies::allow(),
);

let commit_with_same_member = build_validated_commit(Some(true), None, None, None, false);
Expand All @@ -511,6 +535,7 @@ mod tests {
MembershipPolicies::Standard(BasePolicies::Allow),
]),
MembershipPolicies::allow(),
MembershipPolicies::allow(),
);

let member_added_commit = build_validated_commit(Some(true), None, None, None, false);
Expand All @@ -525,6 +550,7 @@ mod tests {
MembershipPolicies::allow(),
]),
MembershipPolicies::allow(),
MembershipPolicies::allow(),
);

let member_added_commit = build_validated_commit(Some(true), None, None, None, false);
Expand All @@ -542,6 +568,7 @@ mod tests {
MembershipPolicies::allow_if_actor_creator(),
MembershipPolicies::deny(),
]),
MembershipPolicies::allow_if_actor_creator(),
);

let proto = permissions.to_proto().unwrap();
Expand All @@ -559,6 +586,7 @@ mod tests {
let permissions = PolicySet::new(
MembershipPolicies::allow_same_member(),
MembershipPolicies::deny(),
MembershipPolicies::allow(),
);

let proto_result = permissions.to_proto();
Expand Down
56 changes: 56 additions & 0 deletions xmtp_mls/src/groups/intents.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@ use xmtp_proto::xmtp::mls::database::{
},
remove_members_data::{Version as RemoveMembersVersion, V1 as RemoveMembersV1},
send_message_data::{Version as SendMessageVersion, V1 as SendMessageV1},
update_metadata_data::{Version as UpdateMetadataVersion, V1 as UpdateMetadataV1},
AccountAddresses, AddMembersData,
AddressesOrInstallationIds as AddressesOrInstallationIdsProtoWrapper, InstallationIds,
PostCommitAction as PostCommitActionProto, RemoveMembersData, SendMessageData,
UpdateMetadataData,
};

use crate::{
Expand Down Expand Up @@ -222,6 +224,50 @@ impl From<RemoveMembersIntentData> for Vec<u8> {
}
}

#[derive(Debug, Clone)]
pub struct UpdateMetadataIntentData {
pub group_name: String,
}

impl UpdateMetadataIntentData {
pub fn new(group_name: String) -> Self {
Self { group_name }
}
}

impl From<UpdateMetadataIntentData> for Vec<u8> {
fn from(intent: UpdateMetadataIntentData) -> Self {
let mut buf = Vec::new();

UpdateMetadataData {
version: Some(UpdateMetadataVersion::V1(UpdateMetadataV1 {
group_name: intent.group_name.clone(),
})),
}
.encode(&mut buf)
.expect("encode error");

buf
}
}

use prost::bytes::Bytes;

impl TryFrom<Vec<u8>> for UpdateMetadataIntentData {
type Error = IntentError;

fn try_from(data: Vec<u8>) -> Result<Self, Self::Error> {
let msg = UpdateMetadataData::decode(Bytes::from(data))?;

let group_name = match msg.version {
Some(UpdateMetadataVersion::V1(ref v1)) => v1.group_name.clone(),
None => return Err(IntentError::Generic("missing payload".to_string())),
};

Ok(Self::new(group_name))
}
}

#[derive(Debug, Clone)]
pub enum PostCommitAction {
SendWelcomes(SendWelcomesAction),
Expand Down Expand Up @@ -363,4 +409,14 @@ mod tests {

assert_eq!(intent.address_or_id, restored_intent.address_or_id);
}

#[tokio::test]
async fn test_serialize_update_metadata() {
let intent = UpdateMetadataIntentData::new("group name".to_string());
let as_bytes: Vec<u8> = intent.clone().try_into().unwrap();
let restored_intent: UpdateMetadataIntentData =
UpdateMetadataIntentData::try_from(as_bytes).unwrap();

assert_eq!(intent.group_name, restored_intent.group_name);
}
}
Loading
Loading