The Websocket client does not provide the functionality usually found in HTTP clients. As a result, it cannot do anything except a straight upgrade from HTTP/1.1 to Websocket. To handle more than this, it would need proprietary configuration to handle: - Authentication - Redirects For reference about the possibilities: https://tyrus.java.net/documentation/1.8/user-guide.html#tyrus-proprietary-config Authentication: https://tyrus.java.net/documentation/1.8/user-guide.html#d0e1524 Redirects: https://tyrus.java.net/documentation/1.8/user-guide.html#d0e1640 This is not a critical enhancement however as users can use their own websocket client implementation, they don't have to rely on the one Tomcat provides.
*** Bug 58623 has been marked as a duplicate of this bug. ***
*** Bug 59191 has been marked as a duplicate of this bug. ***
Hey, I would like to work on this issue if it's ok :) However, as a newbie to tomcat, could you tell me where should I look into? BTW, I had clone and set up tomcat locally and my environment is Ubuntu16.04 Thank you very much!
I'd suggest supporting 302 responses as a starting point. The code should handle both absolute and relative redirects. There is a ready made test case here: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/tomcat/websocket/TestWebSocketFrameClient.java?view=annotate#l100 if you restore the commented out request. The starting point for the client code is here: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java?view=annotate#l341
(In reply to Mark Thomas from comment #4) > I'd suggest supporting 302 responses as a starting point. The code should > handle both absolute and relative redirects. > > There is a ready made test case here: > http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/tomcat/websocket/ > TestWebSocketFrameClient.java?view=annotate#l100 > if you restore the commented out request. > > The starting point for the client code is here: > http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/websocket/ > WsWebSocketContainer.java?view=annotate#l341 Sorry for the late reply. So, how do I know the Websocket client can handle redirections as expected after I make some changes? It's my first bug in Tomcat, so please tell me what should I do or where should I look into? Thank you very much! :)
Re-read my comment #4 regarding a suitable test case and how to activate it.
Created attachment 35193 [details] websocket upgrade redirect Hi, this is my first time submitting any open source contribution. Please let me know if any changes are needed or if anything was missed. Regards
Thanks for the patch. It has been applied to: - trunk for 9.0.0.M26 onwards - 8.5.x for 8.5.20 onwards - 8.0.x for 8.0.46 onwards - 7.0.x for 7.0.80 onwards There were a few minor style things I tweaked. If you enable ChekcStyle and run the validate target it will catch most of them. I also moved the configuration properties from System properties the user properties. Generally, we try to avoid system properties where we can as they can conflict in some use cases. I merged the two properties so redirects are disabled by setting the number of allowed redirects to 0. The default I set to 20 which is consistent with most current browsers. It might look like a lot of changes but they were all fairly minor. Thanks again for the patch.
Where can I learn more about CheckStyle? I assume, there is a formatting file involved. Also, I am interested in adding support for authentication, should I submit a patch to to this thread? Thanks for your time.
Add execute.validate=true to your build.properties file and run ant validate The configuration files are in res/checkstyle. Please open a new bugzilla enhancement for adding authentication support. If you need any pointers, the dev@ list is the place to ask. Thanks again for your contribution and we are looking forward to the next one.
Reopening since authentication is not done and this was for both items, and possibly more.
Created attachment 35289 [details] Authentication support Please find below additional changes. Added support for Basic and Digest Authentication. Added support for dynamic loading of other Authentication Schemes. Cleared redirectSet after invocation to allow for Container reuse.
Ok, so that's obviously the big item (IMO), that looks good. I'm not convinced that digest is useful anymore, do you think it is ? On the plus side, you did it already, on the minus side we'll have to maintain the feature.
(In reply to Remy Maucherat from comment #13) > Ok, so that's obviously the big item (IMO), that looks good. > I'm not convinced that digest is useful anymore, do you think it is ? On the > plus side, you did it already, on the minus side we'll have to maintain the > feature. There is not much need for it nowadays given how ubiquitous SSL is. That being said, it may prove to be useful in certain circumstances. I don't think it should require too much maintenance.
None of the Java classes in the authentication support patch have any Javadoc. I'm -1 on accepting the patch on that basis alone. I've skimmed the code and it otherwise looks good, but I have not tested it at all. For authentication, I wonder how much code can be re-used from Tomcat's existing HTTP Basic and HTTP Digest authenticators. I'd prefer not to have competing implementations of WWW-Authenticate handling in Tomcat.
If there is a possibility of reuse ( this is client side and the existing code is server side) we'd need to be careful about which package / jar we put it in to avoid adding unwanted dependencies for the WebSocket client JAR.
Yes, I would rather integrate it (if it works) then see about reuse. I also don't think javadoc is a big issue either for this kind of code.
I have spent some time looking for opportunities to reuse but did not find many. We could replace the WWWAuthenticate parser for digest with org.apache.tomcat.util.http.parser, but we will still need the one I added for basic. At that point I am not sure if it's worth it if we can't replace it for both schemes.
Are there any additional proposed changes for this patch? I would like to leverage some of the functionality for https://bz.apache.org/bugzilla/show_bug.cgi?id=59758.
I committed the patch to trunk, with a few changes: - Adding javadocs - Merged all client code back to WsWebSocketContainer (the new client class was taking over nearly all its code so I didn't really see the point) If nobody complains about other mandatory improvements, I will proceed with backporting the feature addition.
Wasn't the point of the new class that the redirectSet wasn't thread safe?
Yes, that would be a big problem with my "simplification" then. Ooops. I will restore the separate client class, it's a good solution for the issue.
I prefer getting rid of the field instead, the GC savings are minimal and not worth it IMO.
This should make the enhancement "done" as it adds the most important features. This is included in 9.0.2, 8.5.24, 8.0.48, 7.0.83.
I believe that there are additional benefits for separating the websocket client from the container. For example, we could enhance the redirect flow when behind a proxy by caching the SocketChannel for a connected host:port. We can always, pass the structure as a method argument, but I am not sure that can be manageable long term. In my opinion, as it stands, the container limits flexibility when adding similar features.