Add flexible, framework-invoked authorization logic#647
Conversation
| #[proc_macro_attribute] | ||
| pub fn authorize(_attr: TokenStream, item: TokenStream) -> TokenStream { | ||
| let func: ItemFn = | ||
| syn::parse2(item.clone().into()).expect("#[authorize] must be applied to a function"); | ||
|
|
||
| if let Some(attr) = func | ||
| .attrs | ||
| .iter() | ||
| .find(|attr| attr.path().is_ident("public")) | ||
| { | ||
| return syn::Error::new( | ||
| attr.span(), | ||
| "#[authorize] contradicts #[public]. A public handler must not include authorization.", | ||
| ) | ||
| .to_compile_error() | ||
| .into(); | ||
| } | ||
|
|
||
| item | ||
| } |
There was a problem hiding this comment.
Question on how the two attributes compose, since I want to be sure I'm reading the expansion right. Attribute macros expand top-down and each one consumes its own attribute, so with the documented order
#[get("/email")]
#[authorize(authz::authorize(JsonWebToken))]so #[get] expands first and strips #[authorize] itself. Does this authorize proc macro ever actually run in that case? And if it doesn't, what's enforcing the #[public] conflict for the order users will actually write, can #[get] #[public] #[authorize(...)] compile into a route that's both public and authorized?
There was a problem hiding this comment.
Now the other way around:
#[authorize(authz::authorize(JsonWebToken))]
#[get("/email")]Here authorize expands first and returns item without its own attribute, so by the time #[get] runs there's nothing left to find. Is the route still protected? I don't think it is, and that's the part that worries me. For an auth feature, getting the attribute order wrong shouldn't silently drop the check, it should fail to compile. Could we move all of this into route_macro_core (it already extracts the attr), put the #[public] conflict check there so it actually runs, and make a bare #[authorize] a hard error?
| if let Some(attr) = &contains_authorize { | ||
| let auth_args = attr | ||
| .parse_args::<AuthorizeArgs>() | ||
| .expect("#[authorize] args must be parseable"); |
There was a problem hiding this comment.
Is there a reason we expect() here instead of surfacing a syn::Error? When the attribute args don't parse this panics, and a proc-macro panic renders as proc macro panicked with the message buried under it..
|
|
||
| if normalize_type(ty) == dep_norm { | ||
| let ident = extract_ident(pat)?; | ||
| found = Some(quote!(&#ident)); |
There was a problem hiding this comment.
We hand the dependency to the auth fn two different ways depending on the branch: when a handler param matches, here and when we extract a fresh one, #tmp by value. The auth fn has a single signature for that parameter though, so which one is it meant to be?
| } | ||
|
|
||
| fn normalize_type(ty: &Type) -> String { | ||
| ty.to_token_stream().to_string().replace(' ', "") |
There was a problem hiding this comment.
is this comparison really pulling its weight?
| let args = attr | ||
| .parse_args::<AuthorizeArgs>() | ||
| .expect("#[authorize] args must be parseable"); |
There was a problem hiding this comment.
Why this gets parsed twice? We could parse once and thread it through, minor though.
| content.parse_terminated(Type::parse, Token![,])?; | ||
| parsed.into_iter().collect() | ||
| } else { | ||
| input.parse::<Token![,]>()?; |
There was a problem hiding this comment.
Is the comma form deliberate, or a leftover?
| let (__rapina_parts, _) = __rapina_req.into_parts(); | ||
| #(#header_extractions)* | ||
|
|
||
| //call authorization handler |
There was a problem hiding this comment.
Just a nit, these comments skip the space. fmt won't catch it.
| .state(jwks_client) | ||
| .state(jwks_validation) | ||
| .discover() | ||
| .router(router) |
There was a problem hiding this comment.
| .router(router) |
arferreira
left a comment
arferreira
left a comment
There was a problem hiding this comment.
Nice work @juv! A few things need sorting before it can land.
2 participants
Summary
Adds a new
#[authorize]macro that adds flexible authorization for any endpoint handler.See /examples/jwt-validation for a full example.
Related Issues
Closes #116
Checklist
cargo fmtpassescargo clippyhas no warnings