aremily commented on pull request #99: URL: https://github.com/apache/commons-crypto/pull/99#issuecomment-624355145
@ Vanzin--I think I hit all your points. Can you have another look at this PR? Alex On Fri, May 1, 2020 at 9:42 PM Marcelo Vanzin <[email protected]> wrote: > *@vanzin* commented on this pull request. > > Just minor things. It's a bit weird to see timeouts in these tests. Do > they tend to hang? They shouldn't given what the code is doing. > ------------------------------ > > In > src/test/java/org/apache/commons/crypto/stream/AbstractCipherStreamTest.java > <https://github.com/apache/commons-crypto/pull/99#discussion_r418815328>: > > > @@ -197,46 +224,246 @@ protected void doByteBufferRead(final String cipherClass, final boolean withChan > > getCipher(cipherClass), smallBufferSize, iv, withChannel); > > buf.clear(); > > byteBufferReadCheck(in, buf, 11); > > + in.close(); > > + > > + // Direct buffer, default buffer size, initial buffer position is 0, final read > > > This says default but code says smallBufferSize. Not sure if that > matters... also looking at the code you're adding, it seems a bit redundant > with existing code, but didn't really double-check things. > ------------------------------ > > In > src/test/java/org/apache/commons/crypto/stream/AbstractCipherStreamTest.java > <https://github.com/apache/commons-crypto/pull/99#discussion_r418815800>: > > > + return; // Skip this test if no JNI > > + } > > + } > > + > > + InputStream in = null; > > + OutputStream out = null; > > + try { // Test InvalidAlgorithmParameters > > + in = getCryptoInputStream(transformation, props, new ByteArrayInputStream(encData), > > + new SecretKeySpec(key, "AES"), new GCMParameterSpec(0, new byte[0]), > > + withChannel); > > + Assert.fail("Expected IOException."); > > + } catch (IOException ex) { > > + Assert.assertEquals(ex.getMessage(),"Illegal parameters"); > > + } > > + > > + try { // Test InvalidAlgorithmParameters > > > ⬇️ Suggested change > > - try { // Test InvalidAlgorithmParameters > > + // Test InvalidAlgorithmParameters > > + try { > > > ------------------------------ > > In > src/test/java/org/apache/commons/crypto/stream/AbstractCipherStreamTest.java > <https://github.com/apache/commons-crypto/pull/99#discussion_r418816064>: > > > > > - Assert.assertEquals(dataLen, n1 + n2 + n3); > > + protected void doExceptionTest(final String cipherClass, ByteArrayOutputStream baos, > > + final boolean withChannel) throws IOException { > > + if (AbstractCipherTest.OPENSSL_CIPHER_CLASSNAME.equals(cipherClass)) { > > + if (!Crypto.isNativeCodeLoaded()) { > > + return; // Skip this test if no JNI > > + } > > + } > > + > > + InputStream in = null; > > + OutputStream out = null; > > + try { // Test InvalidAlgorithmParameters > > > ⬇️ Suggested change > > - try { // Test InvalidAlgorithmParameters > > + // Test InvalidAlgorithmParameters > > + try { > > > ------------------------------ > > In > src/test/java/org/apache/commons/crypto/stream/AbstractCipherStreamTest.java > <https://github.com/apache/commons-crypto/pull/99#discussion_r418816208>: > > > + withChannel); > > + Assert.fail("Expected IOException."); > > + } catch (IOException ex) { > > + Assert.assertEquals(ex.getMessage(),"Illegal parameters"); > > + } > > + > > + try { // Test InvalidAlgorithmParameters > > + out = getCryptoOutputStream(transformation, props, baos, > > + new SecretKeySpec(key, "AES"), new GCMParameterSpec(0, > > + new byte[0]), withChannel); > > + Assert.fail("Expected IOException."); > > + } catch (IOException ex) { > > + Assert.assertEquals(ex.getMessage(),"Illegal parameters"); > > + } > > + > > + try { // Test Invalid Key > > > ⬇️ Suggested change > > - try { // Test Invalid Key > > + // Test Invalid Key > > + try { > > > ------------------------------ > > In > src/test/java/org/apache/commons/crypto/stream/AbstractCipherStreamTest.java > <https://github.com/apache/commons-crypto/pull/99#discussion_r418816452>: > > > + new SecretKeySpec(key, "AES"), new GCMParameterSpec(0, > > + new byte[0]), withChannel); > > + Assert.fail("Expected IOException."); > > + } catch (IOException ex) { > > + Assert.assertEquals(ex.getMessage(),"Illegal parameters"); > > + } > > + > > + try { // Test Invalid Key > > + in = getCryptoInputStream(transformation,props, new ByteArrayInputStream(encData), > > + new SecretKeySpec(new byte[10], "AES"), new IvParameterSpec(iv), withChannel); > > + Assert.fail("Expected IOException for Invalid Key"); > > + } catch (IOException ex) { > > + Assert.assertNotNull(ex); > > + } > > + > > + try { // Test Invalid Key > > > ⬇️ Suggested change > > - try { // Test Invalid Key > > + // Test Invalid Key > > + try { > > > ------------------------------ > > In > src/test/java/org/apache/commons/crypto/stream/AbstractCipherStreamTest.java > <https://github.com/apache/commons-crypto/pull/99#discussion_r418816797>: > > > + in = getCryptoInputStream(transformation,props, new ByteArrayInputStream(encData), > > + new SecretKeySpec(new byte[10], "AES"), new IvParameterSpec(iv), withChannel); > > + Assert.fail("Expected IOException for Invalid Key"); > > + } catch (IOException ex) { > > + Assert.assertNotNull(ex); > > + } > > + > > + try { // Test Invalid Key > > + out = getCryptoOutputStream(transformation, props, baos, new byte[10], > > + new IvParameterSpec(iv), withChannel); > > + Assert.fail("Expected IOException for Invalid Key"); > > + } catch (IOException ex) { > > + Assert.assertNotNull(ex); > > + } > > + > > + try { // Test reading a closed stream. > > > I think you get the idea... (the suggestion editor is not very good so > adding those is kinda painful.) > ------------------------------ > > In > src/test/java/org/apache/commons/crypto/stream/AbstractCipherStreamTest.java > <https://github.com/apache/commons-crypto/pull/99#discussion_r418817414>: > > > > > - out.flush(); > > + try { // Test unsupported operation handling. > > + in = getCryptoInputStream(new ByteArrayInputStream(encData), > > + getCipher(cipherClass), defaultBufferSize, iv, false); > > + in.mark(0); // Should not throw an exception. > > > Normally things that should not throw an exception should be outside the > try. (You're also leaving the stream open but that's minor in a test.) > ------------------------------ > > In > src/test/java/org/apache/commons/crypto/stream/AbstractCipherStreamTest.java > <https://github.com/apache/commons-crypto/pull/99#discussion_r418818110>: > > > > > + Assert.assertEquals(out.getOutBuffer().capacity(), defaultBufferSize + 16); > > > Is there a constant for the 16 anywhere? > ------------------------------ > > In > src/test/java/org/apache/commons/crypto/stream/AbstractCipherStreamTest.java > <https://github.com/apache/commons-crypto/pull/99#discussion_r418818937>: > > > @@ -282,6 +570,24 @@ protected CryptoInputStream getCryptoInputStream(final ByteArrayInputStream bais > > return new CryptoInputStream(bais, cipher, bufferSize, > > new SecretKeySpec(key, "AES"), new IvParameterSpec(iv)); > > } > > + > > + protected CryptoInputStream getCryptoInputStream(final String transformation, final Properties props, > > + final ByteArrayInputStream bais, final byte[] key, final AlgorithmParameterSpec params, > > + boolean withChannel) throws IOException { > > + if(withChannel) { > > > ⬇️ Suggested change > > - if(withChannel) { > > + if (withChannel) { > > > But can't you just call the other getCryptoInputStream? > ------------------------------ > > In > src/test/java/org/apache/commons/crypto/stream/AbstractCipherStreamTest.java > <https://github.com/apache/commons-crypto/pull/99#discussion_r418819285>: > > > @@ -294,6 +600,26 @@ protected CryptoOutputStream getCryptoOutputStream( > > return new CryptoOutputStream(baos, cipher, bufferSize, > > new SecretKeySpec(key, "AES"), new IvParameterSpec(iv)); > > } > > + > > + protected CryptoOutputStream getCryptoOutputStream(final String transformation, > > > can just call the other getCryptoOutputStream? > ------------------------------ > > In src/test/java/org/apache/commons/crypto/stream/CtrCryptoStreamTest.java > <https://github.com/apache/commons-crypto/pull/99#discussion_r418819992>: > > > @@ -52,4 +75,115 @@ protected CtrCryptoOutputStream getCryptoOutputStream( > > } > > return new CtrCryptoOutputStream(baos, cipher, bufferSize, key, iv); > > } > > + > > + @Override > > + protected CtrCryptoOutputStream getCryptoOutputStream(final String transformation, > > + final Properties props, final ByteArrayOutputStream baos, final byte[] key, > > + final AlgorithmParameterSpec params, final boolean withChannel) throws IOException { > > + if (withChannel) { > > + return new CtrCryptoOutputStream(props, Channels.newChannel(baos), key, > > + ((IvParameterSpec)params).getIV()); > > + } > > + return new CtrCryptoOutputStream(props, baos, key,((IvParameterSpec)params).getIV()); > > > ⬇️ Suggested change > > - return new CtrCryptoOutputStream(props, baos, key,((IvParameterSpec)params).getIV()); > > + return new CtrCryptoOutputStream(props, baos, key, ((IvParameterSpec)params).getIV()); > > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <https://github.com/apache/commons-crypto/pull/99#pullrequestreview-404488215>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ABUDY7Q52PCRNQYWKCKNVG3RPN3CBANCNFSM4MRJ5JTQ> > . > ---------------------------------------------------------------- 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: [email protected]
