fix(router): provide request context of original WebSocket Upgrade request to subgraph requests#1957
Conversation
|
@SkArchon anything I can do to move this forward? :) |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
WalkthroughA new subtest was added to verify that authentication context can be accessed in header expressions for subgraph requests during WebSocket subscriptions. The subscription execution logic was updated to clone the expression context from the original request context into the subscription request context. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router-tests/websocket_test.go(3 hunks)router/core/websocket.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
router/core/websocket.go (1)
router/internal/expr/expr.go (1)
Context(34-37)
🔇 Additional comments (3)
router-tests/websocket_test.go (3)
4-4: LGTM!The context import is appropriately added to support the new TestAuthenticator implementation.
86-96: LGTM!The
TestAuthenticatorimplementation is clean and appropriate for testing. It correctly implements theauthentication.Authenticatorinterface with predictable claims that can be verified in tests.
863-919: Excellent test implementation!This test case effectively verifies the core functionality described in the PR objectives. It properly:
- Configures header rules with expressions that reference authentication context
- Sets up the test authenticator to provide predictable claims
- Uses global middleware to assert that headers are correctly propagated to subgraph requests
- Follows established testing patterns with proper cleanup and parallel execution
The test directly validates that expression context from the original WebSocket upgrade request is available for header expressions in subgraph requests.
|
Hi @DerZade! I will take a look at this PR and get back to you this week |
…t to corresponding subgraph requests
|
I rebased and added a nil check as CodeRabbit suggested. I'm just still not sure if copying the auth part of the expression context is enough 🤔 I feel like also being able to use other headers (apart from auth) is a benefit. So I think copying the whole context is probably not a bad idea :) |
SkArchon
left a comment
There was a problem hiding this comment.
Thanks for your contribution, it looks good, just one comment. I'll need to check on enabling CI in the meantime as well.
jensneuse
left a comment
There was a problem hiding this comment.
Overall looks like a good PR and useful improvement. Just 2 minor comments.
|
I will open up a separate PR which will run the CI checks, as there are some limitations for CI on forks atm. Also two quick comments,
|
SkArchon
left a comment
There was a problem hiding this comment.
LGTM, will merge after Jen's comments are resolved
|
Addressed all comments :) Please squash merge, since this could easily fit in one commit and there are some commits without nice messages 🙈 |
|
Hi @DerZade we squash merge into main, since we want a clean commit history. In PR commit comments would potentially help reviewing though. Also there is an issue in CI which my colleagues are looking into, so will need to wait until its fixed to merge this in. |
Motivation and Context
Up until now requests to subgraphs that result out of WebSocket subscription request cannot use the expression context of the corresponding
Upgraderequest. This means even if authentication succeeds on theUpgraderequest, the expressionrequest.auth.isAuthenticatedwould evaluate tofalsefor the request to the subgraph. In a similar fashion passing claims to the subgraph via headers is not possible as well.This PR aims to fix this by providing the expression context of the original
Upgraderequest to all resulting requests to subgraphs :)Checklist
Summary by CodeRabbit
New Features
Tests