-
Notifications
You must be signed in to change notification settings - Fork 25
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
Allow opting out of layer auto-reject #16
Conversation
This seems useful. Would this also allow the case of two authentication system (say login with session cookie and JWT based for API clients) using the same handlers without the JWT authorizer blocking all the requests that do not even contain a bearer token header? Assuming the other authentication system has similar optional auth mechanisms of course. |
Also seems great for me - I want to reuse same handlers for authorized and incognito calls, with authorized calls being handled and validated via jwt-authorizer. |
match parts.extensions.get::<Result<TokenData<T>, AuthError>>() { | ||
Some(Ok(data)) => Ok(JwtClaims(data.claims.to_owned())), | ||
Some(Err(e)) => Err(e.to_owned()), | ||
None => { |
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.
I do not have experience with axum extractors - but can we distinguish FromRequestParts<S> for JwtClaims<T>
and FromRequestParts<S> for Option<JwtClaims<T>>
? That way we may define claims to be optional for handlers.
If this is true, but do not relate to the purpose of this PR - please let me know, I will go deeper and create PR with this additional exctractor.
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.
Good question! Axum supports Option
in its extractors, however there is no distinction between extractors that failed because the supplied data is wrong, and those that failed because no data was supplied at all, so the None
case represents 'missing and invalid' which is rarely what you want. Related to this library, an Option<JwtClaims> would not allow you to handle the case where the claims were provided but invalid, you would need to use a Result for that and match the error type to determine which case you want.
I have raised an issue in the past (tokio-rs/axum#1883) and am working on a crate which I plan on finishing up / releasing this week that handles this case. It already supports this crate :)
https://github.com/arlyon/axum-option/blob/main/src/lib.rs#L138-L160
Perhaps it will be made possible by default in the future.
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.
Great work @arlyon - thank you 💜
This prevents the middleware from blanket-rejecting all requests with missing auth, allowing the implementer to set the policy per-handler using extractors.
Updated to address feedback :) |
@arlyon made your PR with recent changes from main branch https://gitlab.com/oss47/jwt-authorizer |
Is that link the canonical home for this repo now? If so I will close out this PR |
@arlyon not yet - we are still discussing where to place actively maintained version of it |
This is semi-stalled, please message here if you are interested in merging. Cheers! |
Hi, for me this was addressed by #31, now you can just extract claims, if no authorization layer is configured to intercept the route there's no rejection. |
This allows a user to opt-in to preventing the middleware from blanket-rejecting all requests with missing auth, allowing the implementer to set the policy per-handler using extractors. This means that for routers that opt out of auto-reject, only the handlers that explicitly request the claims will reject in the case of failure.
This PR also changes the rejection type to AuthError rather than an opaque 500 response, so that people using extractors can better handle errors using
Result<JwtClaim<T>, AuthError>
as an extractor in their handlers (or using my upcoming crate ValidOption which addresses #12)This also parallelizes integration tests by removing global state for the
Stats
struct.