Skip to content
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

Process HTTP session data on websocket handshake and load SecurityContext into Subscriptions #814

Merged
merged 2 commits into from Jan 7, 2022

Conversation

lucatk
Copy link
Contributor

@lucatk lucatk commented Jan 5, 2022

Pull request checklist

  • Please read our contributor guide
  • Consider creating a discussion on the discussion forum
    first
  • Make sure the PR doesn't introduce backward compatibility issues
  • Make sure to have sufficient test cases

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Changes in this PR

This fixes a bug where we don't receive the correct SecurityContext in subscriptions, because the HTTP session etc. is only available during the handshake, meaning Spring Security logic (annotations, etc.) does not work.
The HttpSessionHandshakeInterceptor handles the extraction of HTTP headers during the handshake and makes it available as attributes on the WebSocketSession.
Then the correct SecurityContext is loaded prior to handling any WebSocket message.

Alternatives considered

  • Create own WebSocket controller, but the DgsWebSocketHandler class needs to be open so we can extend it and add the loading of the SecurityContext for ourselves.

@srinivasankavitha
Copy link
Contributor

Thanks for the PR! Are you able to add tests to our existing test suite to verify this functionality?

Copy link
Contributor

@berngp berngp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach seems reasonable, thanks for the fix!

@berngp berngp merged commit 98a7850 into Netflix:master Jan 7, 2022
@lucatk
Copy link
Contributor Author

lucatk commented Jan 10, 2022

Thanks for merging and releasing this ever so quickly!❤️

@LexanRed
Copy link

Thanks for your effort! I am running in the same issues.
Unfortunately, I still get the same problems.

What is the correct way to retrieve the Security Context in a method, called by a subscription?

We use Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
This sometimes works, but not always. I guess there must be a better way.

Any help is very much appreciated 😄

@LexanRed
Copy link

Any input on this one? As described in #450, getting the security context fails sometimes. We managed to figure out, it fails when no mutation / query has been sent before. No session exists in this case.
How do you manage to reliably retrieve the security context?

@Feedcube
Copy link

Thanks a lot for fixing this issue @lucatk !
How did you manage to intercept the handshake to set the authentication object?
When i override the WebSocketConfigurer in an own class to add the interceptor, the registerWebSocketHandlers gets not called

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.

None yet

5 participants