chore(call_server): address post-merge feedback #2474
Conversation
WalkthroughThe changes refactor JWT claim validation logic in the Rust backend, modularizing validation into smaller functions and reorganizing related tests. Documentation comments in the configuration example are clarified. A test variable is renamed for clarity in a separate configuration test, with no changes to public interfaces or configuration formats. Changes
Sequence Diagram(s)sequenceDiagram
participant Config
participant validate_claims
participant validate_claim
participant extract_claim_by_name
participant validate_string_claim
Config->>validate_claims: validate(&self, claims)
validate_claims->>validate_claim: for each expected claim
validate_claim->>extract_claim_by_name: extract claim value
extract_claim_by_name-->>validate_claim: claim value or error
validate_claim->>validate_string_claim: if value is string, validate
validate_string_claim-->>validate_claim: success or error
validate_claim-->>validate_claims: result
validate_claims-->>Config: result
Suggested labels
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The preview of the vlayer book is available at: https://vlayer-book-ndve7kmt3-vlayer.vercel.app |
|
🐢 Incremental Compilation Time: 0:27.52. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rust/server_utils/src/jwt/config.rs (2)
43-49:extract_claim_by_nameneeds proper JSON-Pointer escaping
name.replace(".", "/")assumes every.represents nesting and that claim segments never themselves contain a dot,/, or~.
According to RFC 6901 a JSON-Pointer segment must escape~→~0and/→~1. Using blind replace risks:
- Mis-addressing a claim whose key literally contains a dot.
- Failing on keys that contain
/or~.Diff sketch:
-fn extract_claim_by_name<'a>(name: &'a str, given: &'a Value) -> Result<&'a Value> { - let pointer = format!("/{}", name.replace(".", "/")); +fn extract_claim_by_name<'a>(name: &'a str, given: &'a Value) -> Result<&'a Value> { + let pointer = { + let segments = name.split('.').map(|seg| { + seg.replace('~', "~0").replace('/', "~1") + }); + format!("/{}", segments.collect::<Vec<_>>().join("/")) + };
50-60: Minor simplification opportunity invalidate_string_claimCurrent pattern (
any().then_some(()).ok_or_else(...)) is clever but a bit opaque. A straight boolean guard is simpler and equally performant:- if !expected.values.is_empty() { - expected - .values - .iter() - .any(|exp| exp == given) - .then_some(()) - .ok_or_else(|| unexpected_value(expected, &given))?; - } + if !expected.values.is_empty() + && !expected.values.iter().any(|exp| exp == given) + { + return Err(unexpected_value(expected, &given)); + }Pure readability tweak; feel free to ignore if you prefer the current style.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
book/src/appendix/architecture/prover.md(1 hunks)rust/server_utils/src/jwt/config.rs(2 hunks)rust/services/call/server_lib/src/config.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Test Rust
- GitHub Check: Build binaries
- GitHub Check: Test vlayer
- GitHub Check: Build binaries
- GitHub Check: Build examples
- GitHub Check: Lint TS
- GitHub Check: Lint Rust
- GitHub Check: Rust incremental compilation performance
🔇 Additional comments (3)
rust/services/call/server_lib/src/config.rs (1)
583-590: Good variable-name improvementThe rename from
as_stringtorpc_url_as_stringmakes the intent explicit and removes the vague prefix. No other functional impact – test remains clear. 👍rust/server_utils/src/jwt/config.rs (1)
33-41: Only string claims are supported – call this out explicitly
validate_claimerrors whenever the claim value is not a string, even if the user supplied novaluesfilter. This is fine but should be documented somewhere (docs or error variant) so integrators don’t expect number / bool comparison to work.book/src/appendix/architecture/prover.md (1)
84-90: Docs now accurately describe user-defined JWT claimsThe wording is clearer and no longer over-promises server-side validation. Good catch!
|
Thanks! |
Addresses @AdamDawidKrol's post-merge feedback of #2461 (thanks!)
Summary by CodeRabbit