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

Add Group Metadata #122

Merged
merged 11 commits into from
Jan 11, 2024
Merged

Add Group Metadata #122

merged 11 commits into from
Jan 11, 2024

Conversation

neekolas
Copy link
Collaborator

@neekolas neekolas commented Jan 6, 2024

Summary

  • The proto format for group metadata and associated policies

@neekolas neekolas mentioned this pull request Jan 6, 2024
Merged
@neekolas neekolas marked this pull request as ready for review January 8, 2024 00:09
@neekolas neekolas requested a review from a team as a code owner January 8, 2024 00:09
richardhuaaa
richardhuaaa reviewed Jan 9, 2024
View reviewed changes
proto/mls/message_contents/group_metadata.proto Outdated Show resolved Hide resolved
proto/mls/message_contents/group_metadata.proto Outdated
BASE_POLICY_UNSPECIFIED = 0;
BASE_POLICY_ALLOW = 1;
BASE_POLICY_DENY = 2;
BASE_POLICY_ALLOW_SAME_MEMBER = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

This would only be used for removing members right? Is there any case where it would be permissible for this to NOT be set for removing members?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably not. Think that one would be pretty uniform.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think having this specified as a policy, rather than baked directly into the code, means a malicious person could potentially create a group with this option turned off (no self removal allowed)? I guess my instinct with these changes is to expose the minimum amount for developers to have the features they desire, rather than having maximum flexibility

proto/mls/message_contents/group_metadata.proto Outdated Show resolved Hide resolved
@neekolas
Copy link
Collaborator Author

neekolas commented Jan 9, 2024

@richardhuaaa I've removed the not_condition, since it was unused and has no immediate use-case.

@neekolas
Copy link
Collaborator Author

@richardhuaaa I've removed the Add/Remove installation policies, so the policies are just for members right now.

@neekolas neekolas merged commit 65bf2b2 into main Jan 11, 2024
8 checks passed
@neekolas neekolas deleted the nm/add-policies branch January 11, 2024 23:20
@github-actions GitHub Actions
Copy link

🎉 This PR is included in version 3.36.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardhuaaa richardhuaaa richardhuaaa approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@neekolas @richardhuaaa