-
Notifications
You must be signed in to change notification settings - Fork 102
Let users chose what crypto backend of jsonwebtoken to use #729
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
base: main
Are you sure you want to change the base?
Let users chose what crypto backend of jsonwebtoken to use #729
Conversation
WalkthroughBumps jsonwebtoken to 10.2.0, adds jwt_aws_lc_rs and jwt_rust_crypto feature flags (enables jwt_aws_lc_rs by default), adds Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Cargo.toml(2 hunks)src/tenant_tokens.rs(0 hunks)
💤 Files with no reviewable changes (1)
- src/tenant_tokens.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration-tests
🔇 Additional comments (1)
Cargo.toml (1)
40-40: Good: UUID wasm32 target dependencies correctly include "js" feature.Adding the "js" feature for uuid in wasm32 environments is appropriate for WASM runtimes that lack native random number generation. The feature set ["v4", "js", "serde"] aligns with browser-based contexts.
3378bff to
cc25ab7
Compare
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Cargo.toml (1)
49-50: Both crypto backend features can be enabled simultaneously; add a compile-time guard to prevent user confusion.The claim is verified: jsonwebtoken v10.x requires exactly one crypto backend to be enabled. Currently, nothing prevents users from enabling both
jwt_aws_lc_rsandjwt_rust_cryptoin Cargo.toml simultaneously, which would trigger a compile error from jsonwebtoken itself.Adding the suggested compile-time check in
lib.rsis a good practice to provide early, clear feedback:#[cfg(all(feature = "jwt_aws_lc_rs", feature = "jwt_rust_crypto"))] compile_error!("features 'jwt_aws_lc_rs' and 'jwt_rust_crypto' are mutually exclusive; enable only one");Alternatively, document the constraint prominently in the README so users understand they must choose one backend.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Cargo.toml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration-tests
🔇 Additional comments (2)
Cargo.toml (2)
36-36: ✅ Critical fix applied: hardcoded backend features removed.The previous review flagged that line 36 unconditionally enabled
features = ["aws_lc_rs"], defeating backend choice. This has been correctly fixed by removing the hardcoded features and relying on feature-gating instead.
40-40: Verify wasm32-specific uuid dependency override.Line 40 defines uuid for
wasm32targets with extra features["v4", "js"], while line 29 has the same crate for non-wasm with["v4", "serde"]. Target-specific dependencies override non-target-specific ones, so on wasm32 the serde feature will be lost. Confirm this is intentional or add"serde"to line 40 if needed for consistency.Also applies to: 29-29
Pull Request
What does this PR do?
jsonwebtokenwas updated to 10.x in #717, but there,aws_lc_rswas unconditionally enabled. While I also prefer that backend, this library should probably leave the choice to users. I think my PR achieves exactly that, while still defaulting to aws_lc_rs.PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
Chores
New Features