-
Notifications
You must be signed in to change notification settings - Fork 286
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
feat: allow users to specify a custom header not defined in the struct #420
base: master
Are you sure you want to change the base?
Conversation
jayakasadev
commented
Feb 24, 2025
•
edited
Loading
edited
- allow users to specify custom header values for token generation
- remove deprecated verify_slices_are_equal
- update tests
@jayakasadev , what is the use case for this? |
I have a client which requires that I pass in some custom headers in the token. The |
@jayakasadev , are those headers essential to decoding and validating the token? |
example: https://docs.cdp.coinbase.com/advanced-trade/docs/ws-auth |
@Keats , looks like there is an uncommon, but legit use case for this. |
@rimutaka let me know what changes are needed |
Looks like clippy/cargo fmt needs to be ran |
@jayakasadev , word of caution, I am not a maintainer, so feel free to ignore. The final word is with @Keats. |
understood. I wanted to ask if using a |
I went ahead and implemented the trait approach in a separate branch in case let me know if this approach is more desirable. The usage of |
Immediate thoughts: it's a breaking change and not sure if the existing tests cover all the affected parts. |
understood, im fine abandoning that approach if the performance regression is tolerable
this is on macos arm64 |
@@ -116,7 +116,7 @@ impl<'de> Deserialize<'de> for KeyOperations { | |||
D: Deserializer<'de>, | |||
{ | |||
struct KeyOperationsVisitor; | |||
impl<'de> de::Visitor<'de> for KeyOperationsVisitor { | |||
impl de::Visitor<'_> for KeyOperationsVisitor { |
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.
Why lifetime change?
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.
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.
@jayakasadev, do you know if it's due to the new 1.85.0 elision rules?
I wonder if this change breaks pre-2024 editions.
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 can't say for sure. I don't have visibility into older runs on 1.84.0
or other 1.85.0
runs. If this came up for the first time, then I would hazard to guess that this is the case.
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.
In Rust 2024, all in-scope generic parameters, including lifetime parameters, are implicitly captured when the use<..> bound is not present.
https://doc.rust-lang.org/edition-guide/rust-2024/rpit-lifetime-capture.html
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.
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.
The GH actions run the tests against the MSRV here https://github.com/Keats/jsonwebtoken/blob/master/.github/workflows/ci.yml#L40
Can you manually run that workflow in your fork to see if it works?
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.
like this?
jayakasadev#1
clippy is broken with
I can look into fixing it in this pr, a separate one or mark it as ignored. |
@jayakasadev , I'm not a maintainer and I'm not sure how much Vincent trusts my reviews :) |
thanks @rimutaka |