exceptionfactory commented on code in PR #6032: URL: https://github.com/apache/nifi/pull/6032#discussion_r872569040
########## nifi-nar-bundles/nifi-extension-utils/nifi-event-transport/src/test/resources/netty/client_certificate.cer: ########## @@ -0,0 +1,18 @@ +-----BEGIN CERTIFICATE----- +MIIC3jCCAcagAwIBAgIEYn5khzANBgkqhkiG9w0BAQsFADAvMRwwGgYDVQQKDBNJ +c3N1ZXIgT3JnYW5pemF0aW9uMQ8wDQYDVQQDDAZJc3N1ZXIwIBcNMjIwNTEzMTQw +MDM5WhgPMzAyMTA1MTMxNDAwMzlaMDExHTAbBgNVBAoMFFN1YmplY3QgT3JnYW5p +emF0aW9uMRAwDgYDVQQDDAdTdWJqZWN0MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8A +MIIBCgKCAQEA5I2jZf5rr6nCckKfO/9EZnWkD5AjB2P0P1HJQ3OrjHAmfzZGe6mK +M2pwn+k/dqswkg6lU2F+Ey+SGnK9Y5AJd35nBN3p+jV+BGsxAjxkhdVgOlBK0GyB +E5n9dcXiu9DrFgif6PwPjQe4eshXRpASTrI/ZPaIXf6m8Jy6gahThNEFGmDwRyll +iN/29fglii3v+Ih1eRes2gH6LyRRS1jnVVlbZUvOXK+gqY+YqpAhOAIsdNb3zzJ/ +hGcuIfh4yWQwii9xWzFhinOstHFIOCvdf+DlU5LPS55XYq1rZLzTucwot+f5nDYJ +rvUWzUuqQVGboQfeyuwkbMyoy8SaIeyCpwIDAQABMA0GCSqGSIb3DQEBCwUAA4IB +AQA3Qw+00q7+NKy3jzz+CTDyFehMZvDo8cDO0xGm2qB4hjcR2Sn0oHCbKMZGjwOv +U3Aprqqn9/9+hEVtHj7HmcBM4ocbqAXhdKhKOslPYTt9NRLLyHxrdqPQwQ4AzLGG +GW4kpQQZa1GCaOyXOuAz+LV+vAuFIyvq7PH98yaCIxXrJ2QxhceGZnTtYKSwRnhj +uoG3R7dwumXjiXI7JzhgxX2WQH6qSx1it/PsI4JqghK1ML0xtH/vH+pY9FbODHta +K5FnWnS6B35Qbrei1Irf7LmzZU9C+feZOe0MvtSBrbpcX3BSIHBJgwM179MtwYZM +sFxJtzcHUGnl0K0fwhrs12h+ +-----END CERTIFICATE----- Review Comment: As mentioned, this file should be removed. ########## nifi-nar-bundles/nifi-extension-utils/nifi-event-transport/src/test/java/org/apache/nifi/event/transport/netty/SocketByteArrayMessageDecoderTest.java: ########## @@ -0,0 +1,166 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.nifi.event.transport.netty; + +import io.netty.channel.Channel; +import io.netty.channel.ChannelHandler; +import io.netty.channel.ChannelHandlerContext; +import io.netty.channel.ChannelOutboundHandlerAdapter; +import io.netty.channel.ChannelPipeline; +import io.netty.handler.ssl.SslHandler; +import org.apache.nifi.event.transport.SslSessionStatus; +import org.apache.nifi.event.transport.message.ByteArrayMessage; +import org.apache.nifi.event.transport.netty.codec.SocketByteArrayMessageDecoder; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoExtension; + +import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLPeerUnverifiedException; +import javax.net.ssl.SSLSession; +import javax.security.auth.x500.X500Principal; +import java.io.FileInputStream; +import java.io.InputStream; +import java.net.InetSocketAddress; +import java.security.cert.Certificate; +import java.security.cert.CertificateFactory; +import java.security.cert.X509Certificate; +import java.util.ArrayList; +import java.util.Iterator; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +@ExtendWith(MockitoExtension.class) +public class SocketByteArrayMessageDecoderTest extends SocketByteArrayMessageDecoder { + + private InetSocketAddress inetSocketAddress; + private X509Certificate certificate; + @Mock + private ChannelHandlerContext channelHandlerContext; + @Mock + private Channel channel; + @Mock + private ChannelPipeline channelPipeline; + @Mock + private Iterator<Map.Entry<String, ChannelHandler>> iterator; + @Mock + private Map.Entry<String, ChannelHandler> pipelineEntry; + @Mock + private SslHandler sslHandler; + @Mock + private ChannelOutboundHandlerAdapter nonSslHandler; + @Mock + private SSLEngine sslEngine; + @Mock + private SSLSession sslSession; + + @BeforeEach + public void init() { + inetSocketAddress = new InetSocketAddress("localhost", 21000); + Mockito.when(channelHandlerContext.channel()).thenReturn(channel); + Mockito.when(channel.remoteAddress()).thenReturn(inetSocketAddress); + Mockito.when(channel.pipeline()).thenReturn(channelPipeline); + Mockito.when(channelPipeline.iterator()).thenReturn(iterator); + Mockito.when(iterator.hasNext()).thenReturn(true).thenReturn(true).thenReturn(true).thenReturn(false); + Mockito.when(iterator.next()).thenReturn(pipelineEntry); + } + + @Test + public void testDecodeMessageSsl() throws Exception { + String expectedSubjectDN = "CN=Subject,O=Subject Organization"; + String expectedIssuerDN = "CN=Issuer,O=Issuer Organization"; + + initSsl(); + + List<Object> decoded = new ArrayList<>(1); + decode(channelHandlerContext, "test".getBytes(), decoded); + + X500Principal actualSubject = ((ByteArrayMessage)decoded.get(0)).getSslSessionStatus().getSubject(); + X500Principal actualIssuer = ((ByteArrayMessage)decoded.get(0)).getSslSessionStatus().getIssuer(); + assertEquals(expectedSubjectDN, actualSubject.getName()); + assertEquals(expectedIssuerDN, actualIssuer.getName()); + } + + @Test + public void testDecodeMessageNoSsl() { + initNoSslHandler(); + + List<Object> decoded = new ArrayList<>(1); + decode(channelHandlerContext, "test".getBytes(), decoded); + + SslSessionStatus sslSessionStatus = ((ByteArrayMessage)decoded.get(0)).getSslSessionStatus(); + assertNull(sslSessionStatus); + } + + @Test + public void testDecodeMessageNoCertificates() throws Exception { + initSslWithNoCertificates(); + + List<Object> decoded = new ArrayList<>(1); + decode(channelHandlerContext, "test".getBytes(), decoded); + + SslSessionStatus sslSessionStatus = ((ByteArrayMessage)decoded.get(0)).getSslSessionStatus(); + assertNull(sslSessionStatus); + } + + @Test + public void testDecodeMessagePeerUnverified() throws Exception { + initSslPeerUnverified(); + + List<Object> decoded = new ArrayList<>(1); + decode(channelHandlerContext, "test".getBytes(), decoded); + + SslSessionStatus sslSessionStatus = ((ByteArrayMessage)decoded.get(0)).getSslSessionStatus(); + assertNull(sslSessionStatus); + } + + private void initSsl() throws Exception { + initSslEngine(); + try (InputStream inStream = new FileInputStream("src/test/resources/netty/client_certificate.cer")) { + CertificateFactory cf = CertificateFactory.getInstance("X.509"); + certificate = (X509Certificate)cf.generateCertificate(inStream); + } Review Comment: Static certificate files should not be checked into version control as they will eventually expire and lead to test failures. See `TemporaryKeyStoreBuilder` for an approach to generating certificates for testing. ########## nifi-nar-bundles/nifi-extension-utils/nifi-event-transport/pom.xml: ########## @@ -38,5 +38,9 @@ <version>1.17.0-SNAPSHOT</version> <scope>test</scope> </dependency> + <dependency> + <groupId>junit</groupId> + <artifactId>junit</artifactId> + </dependency> Review Comment: This JUnit 4 dependency should be removed. ########## nifi-nar-bundles/nifi-extension-utils/nifi-event-transport/src/test/java/org/apache/nifi/event/transport/netty/SocketByteArrayMessageDecoderTest.java: ########## @@ -0,0 +1,166 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.nifi.event.transport.netty; + +import io.netty.channel.Channel; +import io.netty.channel.ChannelHandler; +import io.netty.channel.ChannelHandlerContext; +import io.netty.channel.ChannelOutboundHandlerAdapter; +import io.netty.channel.ChannelPipeline; +import io.netty.handler.ssl.SslHandler; +import org.apache.nifi.event.transport.SslSessionStatus; +import org.apache.nifi.event.transport.message.ByteArrayMessage; +import org.apache.nifi.event.transport.netty.codec.SocketByteArrayMessageDecoder; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoExtension; + +import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLPeerUnverifiedException; +import javax.net.ssl.SSLSession; +import javax.security.auth.x500.X500Principal; +import java.io.FileInputStream; +import java.io.InputStream; +import java.net.InetSocketAddress; +import java.security.cert.Certificate; +import java.security.cert.CertificateFactory; +import java.security.cert.X509Certificate; +import java.util.ArrayList; +import java.util.Iterator; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +@ExtendWith(MockitoExtension.class) +public class SocketByteArrayMessageDecoderTest extends SocketByteArrayMessageDecoder { + + private InetSocketAddress inetSocketAddress; + private X509Certificate certificate; + @Mock + private ChannelHandlerContext channelHandlerContext; + @Mock + private Channel channel; + @Mock + private ChannelPipeline channelPipeline; + @Mock + private Iterator<Map.Entry<String, ChannelHandler>> iterator; + @Mock + private Map.Entry<String, ChannelHandler> pipelineEntry; + @Mock + private SslHandler sslHandler; + @Mock + private ChannelOutboundHandlerAdapter nonSslHandler; + @Mock + private SSLEngine sslEngine; + @Mock + private SSLSession sslSession; + + @BeforeEach + public void init() { + inetSocketAddress = new InetSocketAddress("localhost", 21000); + Mockito.when(channelHandlerContext.channel()).thenReturn(channel); + Mockito.when(channel.remoteAddress()).thenReturn(inetSocketAddress); + Mockito.when(channel.pipeline()).thenReturn(channelPipeline); + Mockito.when(channelPipeline.iterator()).thenReturn(iterator); + Mockito.when(iterator.hasNext()).thenReturn(true).thenReturn(true).thenReturn(true).thenReturn(false); + Mockito.when(iterator.next()).thenReturn(pipelineEntry); + } + + @Test + public void testDecodeMessageSsl() throws Exception { + String expectedSubjectDN = "CN=Subject,O=Subject Organization"; + String expectedIssuerDN = "CN=Issuer,O=Issuer Organization"; + + initSsl(); + + List<Object> decoded = new ArrayList<>(1); + decode(channelHandlerContext, "test".getBytes(), decoded); + + X500Principal actualSubject = ((ByteArrayMessage)decoded.get(0)).getSslSessionStatus().getSubject(); + X500Principal actualIssuer = ((ByteArrayMessage)decoded.get(0)).getSslSessionStatus().getIssuer(); + assertEquals(expectedSubjectDN, actualSubject.getName()); + assertEquals(expectedIssuerDN, actualIssuer.getName()); + } + + @Test + public void testDecodeMessageNoSsl() { + initNoSslHandler(); + + List<Object> decoded = new ArrayList<>(1); + decode(channelHandlerContext, "test".getBytes(), decoded); Review Comment: The `"test".getBytes()` method should be declared statically, and `getBytes()` should include the Standard UTF-8 character set. ########## nifi-nar-bundles/nifi-extension-utils/nifi-event-transport/src/test/java/org/apache/nifi/event/transport/netty/SocketByteArrayMessageDecoderTest.java: ########## @@ -0,0 +1,166 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.nifi.event.transport.netty; + +import io.netty.channel.Channel; +import io.netty.channel.ChannelHandler; +import io.netty.channel.ChannelHandlerContext; +import io.netty.channel.ChannelOutboundHandlerAdapter; +import io.netty.channel.ChannelPipeline; +import io.netty.handler.ssl.SslHandler; +import org.apache.nifi.event.transport.SslSessionStatus; +import org.apache.nifi.event.transport.message.ByteArrayMessage; +import org.apache.nifi.event.transport.netty.codec.SocketByteArrayMessageDecoder; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoExtension; + +import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLPeerUnverifiedException; +import javax.net.ssl.SSLSession; +import javax.security.auth.x500.X500Principal; +import java.io.FileInputStream; +import java.io.InputStream; +import java.net.InetSocketAddress; +import java.security.cert.Certificate; +import java.security.cert.CertificateFactory; +import java.security.cert.X509Certificate; +import java.util.ArrayList; +import java.util.Iterator; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +@ExtendWith(MockitoExtension.class) +public class SocketByteArrayMessageDecoderTest extends SocketByteArrayMessageDecoder { + + private InetSocketAddress inetSocketAddress; + private X509Certificate certificate; + @Mock + private ChannelHandlerContext channelHandlerContext; + @Mock + private Channel channel; + @Mock + private ChannelPipeline channelPipeline; + @Mock + private Iterator<Map.Entry<String, ChannelHandler>> iterator; + @Mock + private Map.Entry<String, ChannelHandler> pipelineEntry; + @Mock + private SslHandler sslHandler; + @Mock + private ChannelOutboundHandlerAdapter nonSslHandler; + @Mock + private SSLEngine sslEngine; + @Mock + private SSLSession sslSession; Review Comment: Although this test is narrowly scoped, the number of mocked components makes it difficult to maintain. It would be better to test this class on conjunction with a functional Netty implementation. With the usage of this class tested through TestListenTCP, it seems better to remove this unit test. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
