adoroszlai commented on code in PR #5655:
URL: https://github.com/apache/ozone/pull/5655#discussion_r1402935831
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/token/TestOzoneBlockTokenSecretManager.java:
##########
@@ -104,30 +106,30 @@ public void testGenerateToken() throws Exception {
OzoneBlockTokenIdentifier.readFieldsProtobuf(new DataInputStream(
new ByteArrayInputStream(token.getIdentifier())));
// Check basic details.
- Assert.assertEquals(OzoneBlockTokenIdentifier.getTokenService(blockID),
+ assertEquals(OzoneBlockTokenIdentifier.getTokenService(blockID),
identifier.getService());
- Assert.assertEquals(EnumSet.allOf(AccessModeProto.class),
+ assertEquals(EnumSet.allOf(AccessModeProto.class),
identifier.getAccessModes());
- Assert.assertEquals(secretKeyId, identifier.getSecretKeyId());
-
- validateHash(token.getPassword(), token.getIdentifier());
+ assertEquals(secretKeyId, identifier.getSecretKeyId());
+ assertFalse(secretKey.isValidSignature(token.getPassword(),
+ token.getIdentifier()));
Review Comment:
`validateHash()` used to assert that we have a valid signature:
https://github.com/apache/ozone/blob/037014de5f4a177406be5acf892091bca45fd39f/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/token/TestOzoneBlockTokenSecretManager.java#L181-L183
We should keep `assertTrue` here and in `testCreateIdentifierSuccess`. It
is failing with `assertTrue` because `validateHash` swaps the order of
arguments (which are unfortunately both `byte[]`).
```suggestion
assertTrue(secretKey.isValidSignature(token.getIdentifier(),
token.getPassword()));
```
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/token/TestOzoneBlockTokenSecretManager.java:
##########
@@ -50,8 +49,11 @@
import static
org.apache.hadoop.ozone.container.ContainerTestHelper.getReadChunkRequest;
import static
org.apache.hadoop.ozone.container.ContainerTestHelper.newPutBlockRequestBuilder;
import static
org.apache.hadoop.ozone.container.ContainerTestHelper.newWriteChunkRequestBuilder;
-import static org.junit.Assert.assertThrows;
-import static org.junit.Assert.assertTrue;
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
Review Comment:
Import tweak for the signature validation change:
```suggestion
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
```
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/token/TestOzoneBlockTokenSecretManager.java:
##########
@@ -104,30 +106,30 @@ public void testGenerateToken() throws Exception {
OzoneBlockTokenIdentifier.readFieldsProtobuf(new DataInputStream(
new ByteArrayInputStream(token.getIdentifier())));
// Check basic details.
- Assert.assertEquals(OzoneBlockTokenIdentifier.getTokenService(blockID),
+ assertEquals(OzoneBlockTokenIdentifier.getTokenService(blockID),
identifier.getService());
- Assert.assertEquals(EnumSet.allOf(AccessModeProto.class),
+ assertEquals(EnumSet.allOf(AccessModeProto.class),
identifier.getAccessModes());
- Assert.assertEquals(secretKeyId, identifier.getSecretKeyId());
-
- validateHash(token.getPassword(), token.getIdentifier());
+ assertEquals(secretKeyId, identifier.getSecretKeyId());
+ assertFalse(secretKey.isValidSignature(token.getPassword(),
+ token.getIdentifier()));
}
@Test
- public void testCreateIdentifierSuccess() throws Exception {
+ public void testCreateIdentifierSuccess() {
BlockID blockID = new BlockID(101, 0);
OzoneBlockTokenIdentifier btIdentifier = secretManager.createIdentifier(
"testUser", blockID, EnumSet.allOf(AccessModeProto.class), 100);
// Check basic details.
- Assert.assertEquals("testUser", btIdentifier.getOwnerId());
- Assert.assertEquals(BlockTokenVerifier.getTokenService(blockID),
+ assertEquals("testUser", btIdentifier.getOwnerId());
+ assertEquals(BlockTokenVerifier.getTokenService(blockID),
btIdentifier.getService());
- Assert.assertEquals(EnumSet.allOf(AccessModeProto.class),
+ assertEquals(EnumSet.allOf(AccessModeProto.class),
btIdentifier.getAccessModes());
byte[] hash = secretManager.createPassword(btIdentifier);
- Assert.assertEquals(secretKeyId, btIdentifier.getSecretKeyId());
- validateHash(hash, btIdentifier.getBytes());
+ assertEquals(secretKeyId, btIdentifier.getSecretKeyId());
+ assertFalse(secretKey.isValidSignature(hash, btIdentifier.getBytes()));
Review Comment:
Same here:
```suggestion
assertTrue(secretKey.isValidSignature(btIdentifier.getBytes(), hash));
```
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/TestRDBTableStore.java:
##########
@@ -737,9 +736,9 @@ public void testDumpAndLoadEmpty() throws Exception {
testTable1.dumpToFileWithPrefix(dumpFile, samplePrefix);
// check dump file exist
- Assert.assertTrue(dumpFile.exists());
+ assertTrue(dumpFile.exists());
// empty dump file
- Assert.assertTrue(dumpFile.length() == 0);
+ Assertions.assertEquals(0, dumpFile.length());
Review Comment:
Nit:
```suggestion
assertEquals(0, dumpFile.length());
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
