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

Allow access to underlying WebSocketSession with WebSocketMessageBroker config [SPR-12314] #16919

Closed
spring-issuemaster opened this Issue Oct 8, 2014 · 10 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

spring-issuemaster commented Oct 8, 2014

Rob Winch opened SPR-12314 and commented

It would be nice to provide a hook that allows intercepting the HttpSession id, HttpServletRequest#getUserPrincipal(), and a reference to be able to close the WebSocket Session at the time of the handshake.

This is necessary for things like Spring Session which want to provide a mechanism to override the HttpSession since JSR-356 states:

In the case where a WebSocket endpoint is a protected resource in the web application (see Chapter 8), that is to say, requires an authorized user to access it, then the websocket implementation must ensure that the websocket endpoint does not remain connected to its peer after the underlying implementation has decided the authenticated identity is no longer valid. [WSC-7.2-3] This may happen, for example, if the user logs out
of the containing web application, or if the authentication times out or is invalidated for some other reason.

In this situation, the websocket implementation must immediately close the
close status code 1008. [WSC-7.2-3]

The closest mechanism I could find to do this would be to create a HandshakeHandler. However, there doesn't appear to be a reliable way to refer to an object that can close the WebSocket Session.


Issue Links:

  • #16893 Allow server-side code to send DISCONNECT messages to the broker

Referenced from: commits 97596fb, f0323be

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 9, 2014

Rossen Stoyanchev commented

hey Rob, at the time of handshake there is no WebSocket session yet. It's just an HTTP request to initiate an upgrade. In that sense a HandshakeHandler merely facilitates establishing a session but never "sees" one. That said a HandshakeInterceptor can return false to prevent the handshake and thus the WebSocket session from getting established and it provides access to the request. Have you tried that?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 9, 2014

Rossen Stoyanchev commented

close the WebSocket Session at the time of the handshake

I realize you probably meant get the session at the time of handshake but close it later... doh! One option would be to decorate the WebSocketHandler and thus intercept the afterSessionEstablished callback. That's the first time the application (and the framework) gets a hold of the WebSocket session. I'll set this for 4.1.2 until we clear up there is a good way for Spring Security to do this.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 9, 2014

Rob Winch commented

Rossen Stoyanchev Thanks for the response. Indeed I could have worded that a bit better. You are correct to assume that I do want to close the WebSocket Session later (i.e. when the HttpSession expires or is closed).

I'm wondering if you meant WebSocketHandler#afterConnectionEstablished(WebSocketSession) since I do not see a afterSessionEstablished method?

I looked into this, but I had a few problems:

Obtaining the HttpSession id

I could not find an elegant way to access the HttpSession#getId() from here. One option that would work is to also implement a HandshakeInterceptor and add the HttpSession#getId() to the attributes. Then I can access the HttpSession#getId() with the following code:

NativeWebSocketSession ns = (NativeWebSocketSession) wsSession;
Session session = ns.getNativeSession(Session.class); 
String httpSessionId = (String) session.getUserProperties().get("httpSession.id");

This solution doesn't seem that ideal because:

  • Users must configure both a HandshakeInterceptor and the WebSocketHandler
  • There is quite a bit of casting in this solution. Is this OK? Perhaps there is a better approach?

I can live with these problems, but I'm wondering if there is something I am missing.

Configuration

The second problem was one I haven't gotten past yet. How do you decorate the existing WebSocketHandler? The documentation states how to configure a WebSocketHandler, but it does not appear to have instructions for getting this to work with AbstractWebSocketMessageBrokerConfigurer. Is there a simple way to do this?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 9, 2014

Rossen Stoyanchev commented

After a chat, Rob and I agreed to try the following:

  • update HttpSessionHandshakeInterceptor to allow saving the HTTP session id as a WebSocket session attribute
  • allow decorating the (SubProtocol)WebSocketHandler via WebSocketMessageBrokerConfigurer.configureWebSocketTransport(WebSocketTransportRegistration)

Note the latter will require having a WebSocketHandlerDecoratorFactory.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 13, 2014

Rossen Stoyanchev commented

Rob Winch, the two items from the last comment are now ready. Please give it a try.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 13, 2014

Rob Winch commented

Rossen Stoyanchev I was able to get this to work well. One thing I am still debating is if Spring Session will use the HttpSessionhandshakeInterceptor or provide its own implementation. The reasons:

  • Spring Session does not need any additional session attributes mapped into the WebSocket Session and there is no way to prevent this short of specifying a Single Session Attribute that is unlikely.
  • There are a lot of moving parts to get this to work and I am considering making a single class that implements multiple interfaces to ensure that the correct thing is being done. I still need to see how things flush out before I am convinced this is a good idea.

The take away is...it may be alright (from a Spring Session perspective) to remove the feature for saving the session id from HttpSessionhandshakeInterceptor. Honestly, I see this as something valuable for others, so feel free to leave it too.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 13, 2014

Rossen Stoyanchev commented

Rob, that sounds fine of course. I'm just curious what you mean by "there is no way to prevent this short of specifying a Single Session Attribute that is unlikely". It should be possible to specify no attributes to be copied or is there an issue there?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 13, 2014

Rob Winch commented

I did not try this out, so I could be wrong. However, looking at the source it appears that if CollectionUtils.isEmpty() returns true, then the attribute will be copied. CollectionUtils.isEmpty returns true if the collection is null or empty. So it appears that specifying no attributes means that all attributes are copied.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 14, 2014

Rossen Stoyanchev commented

Good point. I made some improvements. You can now pass an empty collection of attributes and/or set a flag explicitly whether to copy all attributes or not (see the tests.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 14, 2014

Rob Winch commented

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.