Skip to content

feat: allow users to specify a custom header not defined in the struct #420

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

Merged
merged 11 commits into from
May 4, 2025

Conversation

jayakasadev
Copy link
Contributor

@jayakasadev jayakasadev commented Feb 24, 2025

  • allow users to specify custom header values for token generation
  • remove deprecated verify_slices_are_equal
  • update tests

@jayakasadev jayakasadev changed the title allow users to specify a custom header not defined in the struct feat: allow users to specify a custom header not defined in the struct Feb 24, 2025
@rimutaka
Copy link

rimutaka commented Feb 25, 2025

@jayakasadev , what is the use case for this?
Is extra a common name?
I could not find any references or examples in other implementations.

@jayakasadev
Copy link
Contributor Author

I have a client which requires that I pass in some custom headers in the token. The extra field allows users to optionally add such headers.

@rimutaka
Copy link

@jayakasadev , are those headers essential to decoding and validating the token?
Can you provide a real life example?

@jayakasadev
Copy link
Contributor Author

example: https://docs.cdp.coinbase.com/advanced-trade/docs/ws-auth
coinbase's auth requires a nonce header in the jwt token

@rimutaka
Copy link

@Keats , looks like there is an uncommon, but legit use case for this.
Are you open to merging it?
If yes, I'm happy to work with @jayakasadev to get it over the line (docs, probably more tests, code tidy up).

@jayakasadev
Copy link
Contributor Author

@rimutaka let me know what changes are needed

@Keats
Copy link
Owner

Keats commented Mar 1, 2025

Looks like clippy/cargo fmt needs to be ran

@rimutaka
Copy link

rimutaka commented Mar 4, 2025

@jayakasadev , word of caution, I am not a maintainer, so feel free to ignore. The final word is with @Keats.

@jayakasadev
Copy link
Contributor Author

@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 trait would be preferable. This way users who need a new field can define their own struct/impl without impacting existing users.

@jayakasadev
Copy link
Contributor Author

I went ahead and implemented the trait approach in a separate branch in case
jayakasadev@32709ea

let me know if this approach is more desirable. The usage of Hashmap causes a small regression in performance. Traits does not.

@rimutaka
Copy link

rimutaka commented Mar 6, 2025

Immediate thoughts: it's a breaking change and not sure if the existing tests cover all the affected parts.

@jayakasadev
Copy link
Contributor Author

understood, im fine abandoning that approach if the performance regression is tolerable

     Running benches/jwt.rs (target/release/deps/jwt-10e1350ca031353b)
bench_encode            time:   [517.70 ns 520.80 ns 524.03 ns]
                        change: [+0.6197% +1.0974% +1.6803%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) high mild
  1 (1.00%) high severe

bench_decode            time:   [919.45 ns 920.88 ns 922.50 ns]
                        change: [+4.8808% +5.1744% +5.4616%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 5 outliers among 100 measurements (5.00%)
  3 (3.00%) high mild
  2 (2.00%) high severe

this is on macos arm64

@jayakasadev
Copy link
Contributor Author

clippy is broken with

use of deprecated function `ring::deprecated_constant_time::verify_slices_are_equal`: To be removed. Internal function not intended for external use with no promises regarding side channels.
 --> src/crypto/mod.rs:1:26
  |
1 | use ring::constant_time::verify_slices_are_equal;
  |                          ^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `-D deprecated` implied by `-D warnings`
  = help: to override `-D warnings` add `#[allow(deprecated)]`

I can look into fixing it in this pr, a separate one or mark it as ignored.

@jayakasadev jayakasadev requested a review from rimutaka March 7, 2025 15:13
@jayakasadev
Copy link
Contributor Author

@rimutaka @Keats are you still open to this pr being merged?

@rimutaka
Copy link

@jayakasadev , I'm not a maintainer and I'm not sure how much Vincent trusts my reviews :)
I'll give it another good look in the next few days anyway. Most likely early next week.
Thank you for putting in good work. 🙏

@jayakasadev
Copy link
Contributor Author

thanks @rimutaka

@jayakasadev jayakasadev requested a review from Keats March 28, 2025 17:28
@jayakasadev jayakasadev requested a review from Keats April 7, 2025 17:20
@jayakasadev jayakasadev requested a review from Keats April 25, 2025 14:35
@jayakasadev
Copy link
Contributor Author

@Keats thanks for approving
I don't have merge permissions.
The blocking clippy tests are due to deprecated API.

@Keats Keats merged commit 5cd1887 into Keats:master May 4, 2025
2 of 6 checks passed
@Keats
Copy link
Owner

Keats commented May 4, 2025

This is merged but since it's technically a breaking change, I'll wait a bit before releasing it

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.

3 participants