michael-o commented on a change in pull request #300:
URL: https://github.com/apache/maven-surefire/pull/300#discussion_r436655314
##########
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/extensions/SurefireForkChannel.java
##########
@@ -113,6 +124,25 @@ public void connectToClient() throws IOException
}
}
+ private void authenticate() throws InterruptedException,
ExecutionException, IOException
+ {
+ ByteBuffer hash = ByteBuffer.allocate( UUID_STRING_LENGTH );
Review comment:
The name of this variable should be changed.
##########
File path:
maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/extensions/E2ETest.java
##########
@@ -184,4 +192,74 @@ public void close()
.isPositive()
.isLessThanOrEqualTo( 6_000L );
}
+
+ @Test( timeout = 10_000L )
+ public void shouldAuthenticateClient() throws Exception
+ {
+ ForkNodeArguments forkNodeArguments = mock( ForkNodeArguments.class );
+ when( forkNodeArguments.getSessionId() ).thenReturn( UUID.randomUUID()
);
+
+ try ( SurefireForkChannel server = new SurefireForkChannel(
forkNodeArguments ) )
+ {
+ Thread t = new Thread()
+ {
+ @Override
+ public void run()
+ {
+ SurefireMasterProcessChannelProcessorFactory client
+ = new SurefireMasterProcessChannelProcessorFactory();
+ try
+ {
+ client.connect( server.getForkNodeConnectionString() );
+ }
+ catch ( IOException e )
+ {
+ e.printStackTrace();
Review comment:
Wouldn't this be handled anyway when the runtime exception is catched by
JUnit?
##########
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/extensions/SurefireForkChannel.java
##########
@@ -129,7 +159,7 @@ private final void setTrueOptions( SocketOption<Boolean>...
options )
@Override
public String getForkNodeConnectionString()
{
- return "tcp://" + localHost + ":" + localPort;
+ return "tcp://" + localHost + ":" + localPort + ( sessionId == null ?
"" : "?sessionId=" + sessionId );
Review comment:
If this is local host, why do we need it as a vaialble?
##########
File path:
surefire-booter/src/main/java/org/apache/maven/surefire/booter/spi/SurefireMasterProcessChannelProcessorFactory.java
##########
@@ -75,6 +79,12 @@ public void connect( String channelConfig ) throws
IOException
clientSocketChannel = open( withFixedThreadPool( 2,
newDaemonThreadFactory() ) );
setTrueOptions( SO_REUSEADDR, TCP_NODELAY, SO_KEEPALIVE );
clientSocketChannel.connect( hostAddress ).get();
+ UUID sessionId = parseAuth( uri );
+ if ( sessionId != null )
+ {
+ ByteBuffer buff = ByteBuffer.wrap(
sessionId.toString().getBytes( US_ASCII ) );
Review comment:
Think the canonical name is `buf`
##########
File path:
maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/extensions/E2ETest.java
##########
@@ -184,4 +192,74 @@ public void close()
.isPositive()
.isLessThanOrEqualTo( 6_000L );
}
+
+ @Test( timeout = 10_000L )
+ public void shouldAuthenticateClient() throws Exception
+ {
+ ForkNodeArguments forkNodeArguments = mock( ForkNodeArguments.class );
+ when( forkNodeArguments.getSessionId() ).thenReturn( UUID.randomUUID()
);
+
+ try ( SurefireForkChannel server = new SurefireForkChannel(
forkNodeArguments ) )
+ {
+ Thread t = new Thread()
+ {
+ @Override
+ public void run()
+ {
+ SurefireMasterProcessChannelProcessorFactory client
+ = new SurefireMasterProcessChannelProcessorFactory();
+ try
+ {
+ client.connect( server.getForkNodeConnectionString() );
+ }
+ catch ( IOException e )
+ {
+ e.printStackTrace();
+ throw new RuntimeException( e );
+ }
+ }
+ };
+ t.setDaemon( true );
+ t.start();
+ server.connectToClient();
+ }
+ }
+
+ @Test( timeout = 10_000L )
+ public void shouldNotAuthenticateClient() throws Exception
+ {
+ ForkNodeArguments forkNodeArguments = mock( ForkNodeArguments.class );
+ when( forkNodeArguments.getSessionId() ).thenReturn( UUID.randomUUID()
);
+
+ try ( SurefireForkChannel server = new SurefireForkChannel(
forkNodeArguments ) )
+ {
+ Thread t = new Thread()
+ {
+ @Override
+ public void run()
+ {
+ SurefireMasterProcessChannelProcessorFactory client
+ = new SurefireMasterProcessChannelProcessorFactory();
+ try
+ {
+ URI connectionString = new URI(
server.getForkNodeConnectionString() );
+ client.connect( "tcp://" + connectionString.getHost()
+ ":" + connectionString.getPort()
+ +
"?sessionId=6ba7b812-9dad-11d1-80b4-00c04fd430c8" );
+ }
+ catch ( Exception e )
+ {
+ e.printStackTrace();
Review comment:
Same here
##########
File path:
maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/extensions/E2ETest.java
##########
@@ -184,4 +192,74 @@ public void close()
.isPositive()
.isLessThanOrEqualTo( 6_000L );
}
+
+ @Test( timeout = 10_000L )
+ public void shouldAuthenticateClient() throws Exception
+ {
+ ForkNodeArguments forkNodeArguments = mock( ForkNodeArguments.class );
+ when( forkNodeArguments.getSessionId() ).thenReturn( UUID.randomUUID()
);
+
+ try ( SurefireForkChannel server = new SurefireForkChannel(
forkNodeArguments ) )
+ {
+ Thread t = new Thread()
+ {
+ @Override
+ public void run()
+ {
+ SurefireMasterProcessChannelProcessorFactory client
+ = new SurefireMasterProcessChannelProcessorFactory();
+ try
+ {
+ client.connect( server.getForkNodeConnectionString() );
+ }
+ catch ( IOException e )
+ {
+ e.printStackTrace();
+ throw new RuntimeException( e );
+ }
+ }
+ };
+ t.setDaemon( true );
+ t.start();
+ server.connectToClient();
+ }
+ }
+
+ @Test( timeout = 10_000L )
+ public void shouldNotAuthenticateClient() throws Exception
+ {
+ ForkNodeArguments forkNodeArguments = mock( ForkNodeArguments.class );
+ when( forkNodeArguments.getSessionId() ).thenReturn( UUID.randomUUID()
);
+
+ try ( SurefireForkChannel server = new SurefireForkChannel(
forkNodeArguments ) )
+ {
+ Thread t = new Thread()
+ {
+ @Override
+ public void run()
+ {
+ SurefireMasterProcessChannelProcessorFactory client
+ = new SurefireMasterProcessChannelProcessorFactory();
+ try
+ {
+ URI connectionString = new URI(
server.getForkNodeConnectionString() );
+ client.connect( "tcp://" + connectionString.getHost()
+ ":" + connectionString.getPort()
+ +
"?sessionId=6ba7b812-9dad-11d1-80b4-00c04fd430c8" );
+ }
+ catch ( Exception e )
+ {
+ e.printStackTrace();
+ throw new RuntimeException( e );
+ }
+ }
+ };
+ t.setDaemon( true );
+ t.start();
+
+ e.expect( IOException.class );
Review comment:
Stupid question: Don't you need to unwrap from `RuntimeException` first?
##########
File path:
maven-surefire-common/src/test/java/org/apache/maven/surefire/extensions/ForkChannelTest.java
##########
@@ -102,7 +111,8 @@ public ConsoleLogger getConsoleLogger()
String localHost = InetAddress.getLocalHost().getHostAddress();
assertThat( channel.getForkNodeConnectionString() )
.startsWith( "tcp://" + localHost + ":" )
- .isNotEqualTo( "tcp://" + localHost + ":" );
+ .isNotEqualTo( "tcp://" + localHost + ":" )
Review comment:
same here
##########
File path:
maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/extensions/E2ETest.java
##########
@@ -184,4 +192,74 @@ public void close()
.isPositive()
.isLessThanOrEqualTo( 6_000L );
}
+
+ @Test( timeout = 10_000L )
+ public void shouldAuthenticateClient() throws Exception
+ {
+ ForkNodeArguments forkNodeArguments = mock( ForkNodeArguments.class );
+ when( forkNodeArguments.getSessionId() ).thenReturn( UUID.randomUUID()
);
+
+ try ( SurefireForkChannel server = new SurefireForkChannel(
forkNodeArguments ) )
+ {
+ Thread t = new Thread()
+ {
+ @Override
+ public void run()
+ {
+ SurefireMasterProcessChannelProcessorFactory client
+ = new SurefireMasterProcessChannelProcessorFactory();
+ try
+ {
+ client.connect( server.getForkNodeConnectionString() );
+ }
+ catch ( IOException e )
+ {
+ e.printStackTrace();
Review comment:
Sorry, I am blind to see the obvious. Can you please link to the line?
##########
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/extensions/SurefireForkChannel.java
##########
@@ -129,7 +159,7 @@ private final void setTrueOptions( SocketOption<Boolean>...
options )
@Override
public String getForkNodeConnectionString()
{
- return "tcp://" + localHost + ":" + localPort;
+ return "tcp://" + localHost + ":" + localPort + ( sessionId == null ?
"" : "?sessionId=" + sessionId );
Review comment:
I see, but there a few problems here if you want to have maximum
flexibility:
* Just use variables `host` + `port`. Don't imply that you are limited to
`localhost` even if you use it only
* Don't use `Inet4Address`. If the host is running on IPv6 only, the code
will fail. Use `InetAddress` and it will work for both.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
