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

Allow opting out of layer auto-reject #16

Closed
wants to merge 2 commits into from

Conversation

arlyon
Copy link

@arlyon arlyon commented Apr 1, 2023

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.

@arlyon arlyon force-pushed the feat/auto_reject branch from bfa5fb6 to 57cb2ad Compare April 1, 2023 09:45
@taladar
Copy link

taladar commented Jun 9, 2023

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.

@humb1t
Copy link

humb1t commented Jun 19, 2023

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 => {
Copy link

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.

Copy link
Author

@arlyon arlyon Jun 19, 2023

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.

Copy link

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 💜

jwt-authorizer/src/lib.rs Outdated Show resolved Hide resolved
jwt-authorizer/tests/integration_tests.rs Outdated Show resolved Hide resolved
arlyon added 2 commits June 19, 2023 13:16
This prevents the middleware from blanket-rejecting all requests with
missing auth, allowing the implementer to set the policy per-handler
using extractors.
@arlyon arlyon force-pushed the feat/auto_reject branch from 57cb2ad to cfd1cfa Compare June 19, 2023 12:17
@arlyon
Copy link
Author

arlyon commented Jun 19, 2023

Updated to address feedback :)

@humb1t
Copy link

humb1t commented Oct 24, 2023

@arlyon made your PR with recent changes from main branch https://gitlab.com/oss47/jwt-authorizer
Using it to have optional claims in graphql handler - thanks a lot for inspiration!

@arlyon
Copy link
Author

arlyon commented Oct 30, 2023

Is that link the canonical home for this repo now? If so I will close out this PR

@humb1t
Copy link

humb1t commented Oct 30, 2023

@arlyon not yet - we are still discussing where to place actively maintained version of it

@arlyon
Copy link
Author

arlyon commented Jan 4, 2024

This is semi-stalled, please message here if you are interested in merging. Cheers!

@arlyon arlyon closed this Jan 4, 2024
@cduvray
Copy link
Owner

cduvray commented Jan 6, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants