HDDS-15462. Move ACL check in Volume requests to preExecute - #10328
Conversation
sarvekshayr
left a comment
sarvekshayr
left a comment
There was a problem hiding this comment.
Thanks @ss77892 for the patch. LGTM.
|
@ss77892 , there are three different patches for the same "HDDS-13855. Move ACL check in Volume requests to preExecute" JIRA, which is not a practice, we should create individual JIRA for each of them. |
Move ACL authorization checks for volume operations (delete, set-owner, set-quota) and volume ACL operations (add, remove, set ACL) from validateAndUpdateCache to preExecute. This ensures ACL enforcement happens before the Ratis log entry is written, so unauthorized requests are rejected early on the OM leader without producing log entries. Co-authored-by: Cursor <cursoragent@cursor.com>
5af4761 to
cdc81a8
Compare
ss77892
changed the title
ivandika3
left a comment
ivandika3
left a comment
There was a problem hiding this comment.
Thanks for the update. LGTM +1.
| OzoneManagerProtocolProtos.AddAclRequest.Builder addAclRequestBuilder = | ||
| getOmRequest().getAddAclRequest().toBuilder() | ||
| omRequest.getAddAclRequest().toBuilder() | ||
| .setModificationTime(modificationTime); |
There was a problem hiding this comment.
In the future, we also might want to have OMRequest carries the modificationTime so that we are not rebuilding the OMRequest just to add a modificationTime.
|
Thanks @ss77892 for the patch and @ChenSammi for noticing the ticket issue. |
| checkAcls(ozoneManager, OzoneObj.ResourceType.VOLUME, | ||
| OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE_ACL, | ||
| volume, null, null); |
There was a problem hiding this comment.
Seems this is a regression, previously we don't always require WRITE_ACL, but not super.preExecute requires WRITE_ACL. Please check this and write a test for this in another ticket.
4 participants
What changes were proposed in this pull request?
ACL authorization checks for volume operations were moved from validateAndUpdateCache to preExecute in the following request handlers:
• OMVolumeDeleteRequest — DELETE ACL check
• OMVolumeSetOwnerRequest — WRITE_ACL check
• OMVolumeSetQuotaRequest — WRITE check
• OMVolumeAclRequest / OMVolumeAddAclRequest / OMVolumeRemoveAclRequest / OMVolumeSetAclRequest
##What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15462
##How was this patch tested?
• Existing unit tests for TestOMVolumeDeleteRequest, TestOMVolumeSetOwnerRequest, and TestOMVolumeSetQuotaRequest continue to pass.
• Additional integration test in #10331
Made with Cursor