[WebSocket] SSL Support for WebSocket Client. #205
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
irunika
force-pushed
the
websocket-listener-interface-change
branch
from
ce8f4b1 to
1046484
Compare
riyafa
reviewed
Jun 18, 2018
| } | ||
| return Util.getSSLConfigForSender(certPass, keyStorePassword, keyStoreFile, trustStoreFile, trustStorePass, | ||
| parameters, sslProtocol, tlsStoreType); | ||
| } |
There was a problem hiding this comment.
Shall we add javadoc to the public methods?
| @@ -19,31 +19,15 @@ | |||
|
|
|||
| package org.wso2.transport.http.netty.contract.websocket; | |||
|
|
|||
| import java.nio.ByteBuffer; | |||
|
|
|||
| /** | |||
| * This message contains the details of WebSocket bong message. | |||
There was a problem hiding this comment.
I also don't know what a bong message is. Maybe this https://www.discogs.com/artist/7888-Bong-Messages :D
Anyway this is a typo and I will update this java doc with a valid description.
irunika
force-pushed
the
websocket-listener-interface-change
branch
7 times, most recently
from
c092c34 to
d92d208
Compare
Bhashinee
reviewed
Jun 19, 2018
| * @param sslConfig ssl related configurations | ||
| * @return ssl engine | ||
| */ | ||
| private SSLEngine instantiateAndConfigSSL(SSLConfig sslConfig, String host, int port) { |
There was a problem hiding this comment.
Can't we use the same instantiateAndConfigSSL(..) method in Util class making it public?
Bhashinee
reviewed
Jun 19, 2018
| Assert.assertNotNull(throwable); | ||
| Assert.assertTrue(throwable instanceof IllegalArgumentException); | ||
| Assert.assertEquals(throwable.getMessage(), | ||
| "TrustStoreFile or trustStorePassword not defined for HTTPS/WSS_SCHEME scheme"); |
There was a problem hiding this comment.
Should be '... HTTPS/WSS scheme'
Bhashinee
reviewed
Jun 19, 2018
| Assert.assertEquals(throwable.getMessage(), "General SSLEngine problem"); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Shall we remove these 2 empty lines?
Bhashinee
reviewed
Jun 19, 2018
| Assert.assertEquals(clientConnectorListener.getReceivedTextToClient(), testText); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Shall we remove these 2 empty lines?
irunika
force-pushed
the
websocket-listener-interface-change
branch
7 times, most recently
from
e47f3aa to
e2f9183
Compare
Bhashinee
approved these changes
Jun 20, 2018
riyafa
reviewed
Jun 20, 2018
| private void configureHandshakePipeline(ChannelPipeline pipeline) { | ||
| pipeline.addLast(new HttpClientCodec()); | ||
| // Assuming that WebSocket Handshake messages will not be large than 8KB | ||
| pipeline.addLast(new HttpObjectAggregator(8192)); // TODO: Use constant if has |
There was a problem hiding this comment.
Can we remove the TODO here?
| return 443; | ||
| default: | ||
| return -1; | ||
| case "ws": // TODO: Constants |
| this.clientHandshakeFuture = clientHandshakeFuture; | ||
| } | ||
|
|
||
| @Override public void userEventTriggered(ChannelHandlerContext ctx, Object evt) { |
| import static org.wso2.transport.http.netty.util.TestUtil.WEBSOCKET_TEST_IDLE_TIMEOUT; | ||
| import static java.util.concurrent.TimeUnit.SECONDS; | ||
|
|
||
| public class WebSocketSSLHandshakeFailureTestCase { |
There was a problem hiding this comment.
Shall we add a class java doc?
| @@ -82,6 +82,8 @@ | |||
| <class name="org.wso2.transport.http.netty.websocket.client.WebSocketClientHandshakeFunctionalityTestCase"/> | |||
| <class name="org.wso2.transport.http.netty.websocket.client.WebSocketClientFunctionalityTestCase"/> | |||
| <class name="org.wso2.transport.http.netty.websocket.passthrough.WebSocketPassThroughTestCase"/> | |||
| <class name="org.wso2.transport.http.netty.websocket.ssl.WebSocketSSLHandshakeSuccessfulTestCase"/> | |||
| <class name="org.wso2.transport.http.netty.websocket.ssl.WebSocketSSLHandshakeFailureTestCase"/> | |||
There was a problem hiding this comment.
Shall we add the org.wso2.transport.http.netty.websocket package instead of individual classes?
There was a problem hiding this comment.
This should be done as a separate task for all the test cases after a careful inspection.
Created a issue to track this #212
irunika
force-pushed
the
websocket-listener-interface-change
branch
from
e2f9183 to
88a504f
Compare
riyafa
approved these changes
Jun 20, 2018
irunika
force-pushed
the
websocket-listener-interface-change
branch
from
88a504f to
17acfa8
Compare
irunika
force-pushed
the
websocket-listener-interface-change
branch
from
17acfa8 to
9cfb53a
Compare
Bhashinee
pushed a commit
to Bhashinee/transport-http
that referenced
this pull request
Aug 20, 2018
Bhashinee
pushed a commit
to Bhashinee/transport-http
that referenced
this pull request
Aug 20, 2018
…development iteration
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
Goals
Automation tests
Security checks