Copilot commented on code in PR #9192:
URL: https://github.com/apache/ozone/pull/9192#discussion_r2456511060
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMHALeaderSpecificACLEnforcement.java:
##########
@@ -260,7 +287,90 @@ private void testVolumeAndBucketCreationAsUser(boolean
shouldSucceed) throws Exc
}
} finally {
// Reset to original user
- UserGroupInformation.setLoginUser(UserGroupInformation.getCurrentUser());
+ UserGroupInformation.setLoginUser(adminUserUgi);
+ }
+ }
+
+ /**
+ * Tests that setTimes ACL check is enforced in preExecute and is
leader-specific.
+ * 1. Creates a key with admin user
+ * 2. Adds test user as admin on the current leader
+ * 3. Verifies that test user (as admin) can setTimes on key owned by
someone else
+ * 4. Transfers leadership to another node
+ * 5. Verifies that setTimes fails with PERMISSION_DENIED when test user is
no longer admin
+ */
+ @Test
+ public void testKeySetTimesAclEnforcementAfterLeadershipChange() throws
Exception {
+ // Step 1: Create a volume, bucket, and key as the admin user
+ ObjectStore adminObjectStore = client.getObjectStore();
+ String keyTestVolume = "keyvol-" +
+ RandomStringUtils.secure().nextAlphabetic(5).toLowerCase(Locale.ROOT);
+ String keyTestBucket = "keybucket-" +
+ RandomStringUtils.secure().nextAlphabetic(5).toLowerCase(Locale.ROOT);
+ String keyName = "testkey-" +
+ RandomStringUtils.secure().nextAlphabetic(5).toLowerCase(Locale.ROOT);
+
+ String adminUser = adminUserUgi.getShortUserName();
+ VolumeArgs volumeArgs = VolumeArgs.newBuilder()
+ .setOwner(adminUser)
+ .build();
+ adminObjectStore.createVolume(keyTestVolume, volumeArgs);
+ OzoneVolume adminVolume = adminObjectStore.getVolume(keyTestVolume);
+
+ BucketArgs bucketArgs = BucketArgs.newBuilder().build();
+ adminVolume.createBucket(keyTestBucket, bucketArgs);
+ OzoneBucket adminBucket = adminVolume.getBucket(keyTestBucket);
+
+ // Create a key as admin (so test user is NOT the owner)
+ try (OzoneOutputStream out = adminBucket.createKey(keyName, 0)) {
+ out.write("test data".getBytes(DEFAULT_ENCODING));
+ }
+
+ OzoneKey key = adminBucket.getKey(keyName);
+ assertNotNull(key, "Key should be created successfully");
+ long originalMtime = key.getModificationTime().toEpochMilli();
+
+ // Step 2: Get the current leader and add test user as admin
+ OzoneManager currentLeader = cluster.getOMLeader();
+ String leaderNodeId = currentLeader.getOMNodeId();
+ addAdminToSpecificOM(currentLeader, TEST_USER);
+
+ // Verify admin was added
+ assertTrue(currentLeader.getOmAdminUsernames().contains(TEST_USER),
+ "Test user should be admin on leader OM");
+
+ // Switch to test user and try setTimes as admin (should succeed)
+ UserGroupInformation.setLoginUser(testUserUgi);
+ try (OzoneClient userClient =
OzoneClientFactory.getRpcClient(OM_SERVICE_ID, cluster.getConf())) {
+ ObjectStore userObjectStore = userClient.getObjectStore();
+ OzoneVolume userVolume = userObjectStore.getVolume(keyTestVolume);
+ OzoneBucket userBucket = userVolume.getBucket(keyTestBucket);
+
+ long newMtime = System.currentTimeMillis();
+ userBucket.setTimes(keyName, newMtime, -1);
+
+ // Verify the modification time was updated
+ key = userBucket.getKey(keyName);
+ assertEquals(newMtime, key.getModificationTime().toEpochMilli(),
+ "Modification time should be updated by admin user");
+ assertNotEquals(originalMtime, key.getModificationTime().toEpochMilli(),
+ "Modification time should have changed");
+
+ OzoneManager newLeader = transferLeadershipToAnotherNode(currentLeader);
+ assertNotEquals(leaderNodeId, newLeader.getOMNodeId(),
+ "Leadership should have transferred to a different node");
+ assertFalse(newLeader.getOmAdminUsernames().contains(TEST_USER),
+ "Test user should NOT be admin on new leader OM");
+ Thread.sleep(2000);
Review Comment:
Use `GenericTestUtils.waitFor()` instead of `Thread.sleep()` to wait for the
leadership change to propagate and ACL configuration to stabilize. This makes
the test more reliable and faster by polling for the actual condition rather
than using arbitrary sleep duration.
```suggestion
GenericTestUtils.waitFor(
() -> !newLeader.getOmAdminUsernames().contains(TEST_USER),
100, // poll interval ms
10_000 // timeout ms
);
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMHALeaderSpecificACLEnforcement.java:
##########
@@ -260,7 +287,90 @@ private void testVolumeAndBucketCreationAsUser(boolean
shouldSucceed) throws Exc
}
} finally {
// Reset to original user
- UserGroupInformation.setLoginUser(UserGroupInformation.getCurrentUser());
+ UserGroupInformation.setLoginUser(adminUserUgi);
+ }
+ }
+
+ /**
+ * Tests that setTimes ACL check is enforced in preExecute and is
leader-specific.
+ * 1. Creates a key with admin user
+ * 2. Adds test user as admin on the current leader
+ * 3. Verifies that test user (as admin) can setTimes on key owned by
someone else
+ * 4. Transfers leadership to another node
+ * 5. Verifies that setTimes fails with PERMISSION_DENIED when test user is
no longer admin
+ */
+ @Test
+ public void testKeySetTimesAclEnforcementAfterLeadershipChange() throws
Exception {
+ // Step 1: Create a volume, bucket, and key as the admin user
+ ObjectStore adminObjectStore = client.getObjectStore();
+ String keyTestVolume = "keyvol-" +
+ RandomStringUtils.secure().nextAlphabetic(5).toLowerCase(Locale.ROOT);
+ String keyTestBucket = "keybucket-" +
+ RandomStringUtils.secure().nextAlphabetic(5).toLowerCase(Locale.ROOT);
+ String keyName = "testkey-" +
+ RandomStringUtils.secure().nextAlphabetic(5).toLowerCase(Locale.ROOT);
+
+ String adminUser = adminUserUgi.getShortUserName();
+ VolumeArgs volumeArgs = VolumeArgs.newBuilder()
+ .setOwner(adminUser)
+ .build();
+ adminObjectStore.createVolume(keyTestVolume, volumeArgs);
+ OzoneVolume adminVolume = adminObjectStore.getVolume(keyTestVolume);
+
+ BucketArgs bucketArgs = BucketArgs.newBuilder().build();
+ adminVolume.createBucket(keyTestBucket, bucketArgs);
+ OzoneBucket adminBucket = adminVolume.getBucket(keyTestBucket);
+
+ // Create a key as admin (so test user is NOT the owner)
+ try (OzoneOutputStream out = adminBucket.createKey(keyName, 0)) {
+ out.write("test data".getBytes(DEFAULT_ENCODING));
Review Comment:
The `getBytes(DEFAULT_ENCODING)` call uses the string name of the encoding
which can throw `UnsupportedEncodingException`. Use `getBytes(UTF_8)` directly
instead, where `UTF_8` is the `Charset` constant from
`java.nio.charset.StandardCharsets`, which cannot throw a checked exception.
```suggestion
out.write("test data".getBytes(UTF_8));
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeySetTimesRequest.java:
##########
@@ -78,6 +78,27 @@ public OMRequest preExecute(OzoneManager ozoneManager)
throws IOException {
OzoneManagerProtocolProtos.KeyArgs newKeyArgs =
resolveBucketLink(ozoneManager, keyArgs);
+ // ACL check during preExecute
+ if (ozoneManager.getAclsEnabled()) {
+ try {
+ checkAcls(ozoneManager, OzoneObj.ResourceType.KEY,
+ OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE_ACL,
+ newKeyArgs.getVolumeName(), newKeyArgs.getBucketName(),
newKeyArgs.getKeyName());
+ } catch (IOException ex) {
+ // Ensure audit log captures preExecute failures
+ Map<String, String> auditMap = new LinkedHashMap<>();
+ auditMap.put(OzoneConsts.VOLUME, newKeyArgs.getVolumeName());
+ auditMap.put(OzoneConsts.BUCKET, newKeyArgs.getBucketName());
+ auditMap.put(OzoneConsts.KEY, newKeyArgs.getKeyName());
+ auditMap.put(OzoneConsts.MODIFICATION_TIME,
+ String.valueOf(getModificationTime()));
+ markForAudit(ozoneManager.getAuditLogger(),
+ buildAuditMessage(OMAction.SET_TIMES, auditMap, ex,
+ getOmRequest().getUserInfo()));
+ throw ex;
+ }
+ }
Review Comment:
[nitpick] The audit map construction for ACL check failures duplicates logic
that likely exists in validateAndUpdateCache. Consider extracting a shared
method `buildSetTimesAuditMap()` to avoid duplication and ensure consistency in
audit logging across both code paths.
--
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]
