Copilot commented on code in PR #10753:
URL: https://github.com/apache/ozone/pull/10753#discussion_r3578195656
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/lifecycle/OMLifecycleConfigurationDeleteRequest.java:
##########
@@ -175,6 +174,22 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager, Execut
}
}
+ private void checkAclPermission(OzoneManager ozoneManager, String
volumeName, String bucketName)
+ throws IOException {
+ if (ozoneManager.getAccessAuthorizer().isNative()) {
+ UserGroupInformation ugi = createUGIForApi();
+ String bucketOwner = ozoneManager.getBucketOwner(volumeName, bucketName,
+ IAccessAuthorizer.ACLType.READ, OzoneObj.ResourceType.BUCKET);
+ if (!ozoneManager.isAdmin(ugi) && !ozoneManager.isOwner(ugi,
bucketOwner)) {
+ throw new OMException("Lifecycle configuration can only be deleted by
bucket Admin or Owner",
+ OMException.ResultCodes.PERMISSION_DENIED);
+ }
Review Comment:
The permission check enforces cluster admin via ozoneManager.isAdmin(ugi),
but the error message says "bucket Admin" which can be misread as a
bucket-level role. Align the message with the actual check to avoid confusion
for clients and operators.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/lifecycle/OMLifecycleConfigurationSetRequest.java:
##########
@@ -86,9 +87,7 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws
IOException {
Pair.of(volumeName, bucketName), this);
if (ozoneManager.getAclsEnabled()) {
- checkAcls(ozoneManager, OzoneObj.ResourceType.BUCKET,
OzoneObj.StoreType.OZONE,
- IAccessAuthorizer.ACLType.ALL, resolvedBucket.realVolume(),
- resolvedBucket.realBucket(), null);
+ checkAclPermission(ozoneManager, resolvedBucket.realVolume(),
resolvedBucket.realBucket());
}
Review Comment:
This change introduces split authorization semantics (native: admin/owner;
non-native: WRITE ACL) but the existing unit tests for lifecycle configuration
requests never enable ACLs, so the new branches are not exercised. Add tests
that set getAclsEnabled()=true and verify: (1) non-native authorizer triggers a
WRITE bucket ACL check, and (2) native authorizer denies non-admin/non-owner
and allows admin or owner.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/lifecycle/OMLifecycleConfigurationSetRequest.java:
##########
@@ -204,6 +203,22 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager, Execut
}
}
+ private void checkAclPermission(OzoneManager ozoneManager, String
volumeName, String bucketName)
+ throws IOException {
+ if (ozoneManager.getAccessAuthorizer().isNative()) {
+ UserGroupInformation ugi = createUGIForApi();
+ String bucketOwner = ozoneManager.getBucketOwner(volumeName, bucketName,
+ IAccessAuthorizer.ACLType.READ, OzoneObj.ResourceType.BUCKET);
+ if (!ozoneManager.isAdmin(ugi) && !ozoneManager.isOwner(ugi,
bucketOwner)) {
+ throw new OMException("Lifecycle configuration can only be set by
bucket Admin or Owner",
+ OMException.ResultCodes.PERMISSION_DENIED);
+ }
Review Comment:
The permission check enforces cluster admin via ozoneManager.isAdmin(ugi),
but the error message says "bucket Admin" which can be misread as a
bucket-level role. Align the message with the actual check to avoid confusion
for clients and operators.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/lifecycle/OMLifecycleConfigurationDeleteRequest.java:
##########
@@ -77,9 +78,7 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws
IOException {
Pair.of(volumeName, bucketName), this);
if (ozoneManager.getAclsEnabled()) {
- checkAcls(ozoneManager, OzoneObj.ResourceType.BUCKET,
OzoneObj.StoreType.OZONE,
- IAccessAuthorizer.ACLType.ALL, resolvedBucket.realVolume(),
- resolvedBucket.realBucket(), null);
+ checkAclPermission(ozoneManager, resolvedBucket.realVolume(),
resolvedBucket.realBucket());
}
Review Comment:
This change introduces split authorization semantics (native: admin/owner;
non-native: WRITE ACL) but the existing unit tests for lifecycle configuration
requests never enable ACLs, so the new branches are not exercised. Add tests
that set getAclsEnabled()=true and verify: (1) non-native authorizer triggers a
WRITE bucket ACL check, and (2) native authorizer denies non-admin/non-owner
and allows admin or owner.
--
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]
