feat(hermes-server): add optional token to all apis and track usage#3404
feat(hermes-server): add optional token to all apis and track usage#3404keyvankhademi merged 9 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
4 Skipped Deployments
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d8c9a25c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
apps/hermes/server/src/api/token.rs
Outdated
| if let Ok(auth_str) = auth_header.to_str() { | ||
| if let Some(token) = auth_str.strip_prefix("Bearer ") { | ||
| let token = token.trim(); |
There was a problem hiding this comment.
Handle lowercase bearer in Authorization header
In extract_token_from_headers_and_uri, the token is only recognized when the header starts with the exact string Bearer , but the auth scheme is case-insensitive per RFC 7235; clients that send bearer <token> (some HTTP libraries do) will be treated as unauthenticated and recorded as none, which defeats the new usage tracking for those clients. Consider normalizing the scheme or performing a case-insensitive match before extracting the token.
Useful? React with 👍 / 👎.
ali-behjati
left a comment
There was a problem hiding this comment.
Overall LGTM, please fix my comments before merging. Also bump the Hermes Server Cargo.toml file.
We also have a price service client js, and many websocket users over TS use it. Can you update that too?
In the future we might need to track the usage of tokens per asset or per asset class.
| this.appendUrlSearchParams(url, transformedOptions); | ||
| } | ||
|
|
||
| // Add access token as query param for SSE connections |
There was a problem hiding this comment.
Does SSE also require query param here? I thought that was only WS
There was a problem hiding this comment.
Latency itself has a lot of params, we don't need it per API key.
…ing in PriceServiceConnection
Summary
add optional token to all apis and track usage
Rationale
We need to accept the token to allow users to add it on their side before enforcing it. Also tracking users.
How has this been tested?
Manually tested by setting api key and looking at the /metrics