feat: String based permission nodes #632
Conversation
There was a problem hiding this comment.
Could you explain how this has benefits over just using enums, given that the permissions themselves are fixed, having them as strings altogether seems a bit redundant. Internally when is serialized to a string, it can represent as this node-style and it could be easily deserialize to an enum.
tools/enum_macro/src/lib.rs
Outdated
| /// } | ||
| /// | ||
| /// assert_eq!(SomeEnum::SomeValue1.to_string(), "prefix.some_value1"); | ||
| /// assert_eq!(SomeEnum::SomeValue2.to_string(), "some_value2"); | ||
| /// ``` | ||
| #[proc_macro_derive(EnumFormatting, attributes(permission))] | ||
| pub fn permission_node(input: TokenStream) -> TokenStream { | ||
| let input = parse_macro_input!(input as DeriveInput); | ||
| let name = &input.ident; | ||
| if let Data::Enum(enum_data) = &input.data { | ||
| let mut variants = vec![]; | ||
| for variant in &enum_data.variants { | ||
| let mut format = None; | ||
| if let Some(attr) = variant | ||
| .attrs | ||
| .iter() | ||
| .find(|attr| attr.path().is_ident("permission")) | ||
| { | ||
| if let syn::Meta::List(l) = &attr.meta { | ||
| if let Err(err) = l.parse_nested_meta(|meta| { | ||
| if meta.path.is_ident("formatting") { | ||
| let exp: syn::Expr = meta.value()?.parse()?; | ||
| if let syn::Expr::Lit(v) = exp { | ||
| if let syn::Lit::Str(s) = v.lit { | ||
| format = Some(s.value()); | ||
| } | ||
| } | ||
| return Ok(()); | ||
| } | ||
| Err(meta.error("Unrecognized attribute")) | ||
| }) { | ||
| panic!("{:?}", err); | ||
| } | ||
| } | ||
| } | ||
| let format = format.unwrap_or("{}".into()); | ||
|
|
||
| let variant_name = &variant.ident; | ||
|
|
||
| variants.push(quote! { | ||
| #name::#variant_name => { | ||
| let mut snake = String::new(); | ||
| for (i, ch) in stringify!(#variant_name).char_indices() { | ||
| if i > 0 && ch.is_uppercase() { | ||
| snake.push('_'); | ||
| } | ||
| snake.push(ch.to_ascii_lowercase()); | ||
| } | ||
| format!(#format, snake) | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| let expanded = quote! { | ||
| impl std::fmt::Display for #name { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| let result = match self { | ||
| #(#variants)* | ||
| }; | ||
| f.write_str(&format!("{}", result)) | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| TokenStream::from(expanded) | ||
| } else { | ||
| panic!("Only Enums are supported"); | ||
| } | ||
| } | ||
|
|
||
| /// Implements #values() for enum values that returns all defined values in the enum | ||
| /// E.g. | ||
| /// ``` | ||
| /// use enum_macro::EnumValues; | ||
| /// #[derive(EnumValues, PartialEq, Debug)] | ||
| /// pub enum SomeEnum { | ||
| /// SomeValue1, | ||
| /// SomeValue2 | ||
| /// } | ||
| /// | ||
| /// assert_eq!([SomeEnum::SomeValue1, SomeEnum::SomeValue2], SomeEnum::values()); | ||
| /// ``` | ||
| #[proc_macro_derive(EnumValues)] | ||
| pub fn enum_values(input: TokenStream) -> TokenStream { | ||
| let input = parse_macro_input!(input as DeriveInput); | ||
| let name = &input.ident; | ||
| if let Data::Enum(enum_data) = input.data { | ||
| let variants: Vec<_> = enum_data.variants.into_iter().map(|v| v.ident).collect(); | ||
| let expanded = quote! { | ||
| impl #name { | ||
| pub fn values() -> &'static[#name] { | ||
| &[ #(#name::#variants),* ] | ||
| } | ||
| } | ||
| }; | ||
| TokenStream::from(expanded) | ||
| } else { | ||
| panic!("Only Enums are supported"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Wouldnt it be better to just use serde to rename the enum than to use a macro?
There was a problem hiding this comment.
serde can only to something like upper/lowercase etc. they dont have anything like formatting sadly
There was a problem hiding this comment.
you can use #[serde(rename="some.permission.node")] for each enum variant. Ie
#[derive(Serialize, Deserialize)]
enum PermissionNode {
#[serde(rename="add.identity")]
AddIdentity,
#[serde(rename="remove.identity")]
RemoveIdentity
}There was a problem hiding this comment.
yes but with that its a bit decoupled from the actual enum name which i didnt like
There was a problem hiding this comment.
Thats only an example though. It could be named anything, however serde would handle that though when it comes to deserializing it back to the enum.
There was a problem hiding this comment.
i meant that serde does not give access to the enum name inside renaming. so if you e.g. rename the enum itself AddIdentity you also need to update the serialized name. its a bit nitpicky but initially i wanted the enums purely for sanity checks within warp and allow arbitrary strings as permissions
There was a problem hiding this comment.
In that case you would want to serialize it manually if you dont want to update the rename attr macro, although this shouldnt exactly be a problem to rename it either (though to take caution against breaking changes too).
There was a problem hiding this comment.
i can change it back to serde if you think its better. i personally think manually defining the string everytime is a bit cumbersome
initially it was so you could set (and check) arbitrary permission as otherwise they would be hardcoded. It would allow other devs etc. to make use of this system too and defining their own permissions. though i agree that the way its currently written still doesnt allow that |
|
hmm not sure why running |
|
Test failing due to invalid field (should be EDIT: Looks like the event originally had |
What this PR does 📖
Rewrites the community permission system to use string based permissions instead of enums. String based permission have the form
some.permissionand have the advantage that they easily expandable and also allows hierarchy checking.When granting/revoking
someit will grant all permissions under the same namespace e.g.some.permission1andsome.permission2.The builtin permissions are still in an enum to allow ease of usage. The API to check a permission accepts anything that is representable as a String.
Also adds
has_community_permissionandhas_community_channel_permissionto the API