Bug 57767 - Websocket client proprietary configuration
Summary: Websocket client proprietary configuration
Status: REOPENED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: WebSocket (show other bugs)
Version: unspecified
Hardware: PC Linux
: P1 enhancement (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
: 58623 59191 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-03-27 14:53 UTC by Remy Maucherat
Modified: 2017-08-03 08:01 UTC (History)
2 users (show)



Attachments
websocket upgrade redirect (8.57 KB, patch)
2017-08-01 02:42 UTC, J Fernandez
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Remy Maucherat 2015-03-27 14:53:32 UTC
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.
Comment 1 Mark Thomas 2015-11-22 16:53:28 UTC
*** Bug 58623 has been marked as a duplicate of this bug. ***
Comment 2 Remy Maucherat 2016-03-17 16:53:07 UTC
*** Bug 59191 has been marked as a duplicate of this bug. ***
Comment 3 MikeLing 2016-09-25 03:34:19 UTC
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!
Comment 4 Mark Thomas 2016-09-25 09:07:50 UTC
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
Comment 5 MikeLing 2016-09-28 16:06:03 UTC
(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! :)
Comment 6 Mark Thomas 2016-09-29 12:58:53 UTC
Re-read my comment #4 regarding a suitable test case and how to activate it.
Comment 7 J Fernandez 2017-08-01 02:42:35 UTC
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
Comment 8 Mark Thomas 2017-08-02 19:12:02 UTC
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.
Comment 9 J Fernandez 2017-08-02 22:04:13 UTC
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.
Comment 10 Mark Thomas 2017-08-02 22:08:13 UTC
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.
Comment 11 Remy Maucherat 2017-08-03 07:41:19 UTC
Reopening since authentication is not done and this was for both items, and possibly more.