-
Notifications
You must be signed in to change notification settings - Fork 593
HDDS-13932. Add test cases for PUT bucket ACL #9371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Gargi-jais11
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Russole for the patch.
Left some comments below.
|
|
||
| assertEquals("InvalidRequest", os3.getCode()); | ||
| assertEquals(400, os3.getHttpCode()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here also we should use constant references consistently.
| assertEquals("InvalidRequest", os3.getCode()); | |
| assertEquals(400, os3.getHttpCode()); | |
| assertEquals(INVALID_REQUEST.getCode(), os3.getCode());; | |
| assertEquals(400, os3.getHttpCode()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. I've updated the code to use constant references consistently.
| assertEquals(HTTP_NOT_FOUND, ex.getHttpCode()); | ||
| assertEquals(MALFORMED_HEADER.getCode(), ex.getCode()); | ||
| } | ||
| public void testAclWithMissingHeadersAndNoBody() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also have a test scenario where both headers and body are present, and check headers take precedence and the body is ignored.
Verify that the permissions set on the bucket is as per header and not according to the body.
testPutAclWithBothHeadersAndBody()
- Mock header with one permission
- Provide body with different permission (should be ignored)
- Verify that READ permission was set (from header), not FULL_CONTROL (from body)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, I have added a new test to cover this scenario:
In this test:
- The header grants READ via x-amz-grant-read (id=owner-id),
- The XML body (VALID_ACL_XML) grants FULL_CONTROL,
- The call succeeds with HTTP 200, and We verify that an ACL entry for owner-id with ACCESS scope is present on the bucket, which is derived from the header.
Since putAcl() only takes the header-based path when any grant header is present (and skips the body in that case), this test documents that the header takes precedence over the body when both are provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case ...MissingHeadersAndNoBody is no longer necessary.
|
Few other scenarios are also possible, so do you have idea what will be the behaviour in the below scenarios?:
So in the above two case what should happen, create bucket should pass or fail ? And according do testing of these two scenarios as well. |
Thanks for pointing out these additional edge cases. I checked the current implementation of BucketEndpoint.putAcl() (in particular getAndConvertAclOnBucket / getAndConvertAclOnVolume) and added tests to document the behavior for both scenarios:
when(mockHeaders.getHeaderString(S3Acl.GRANT_FULL_CONTROL))
.thenReturn("");
Response resp = bucketEndpoint.put(bucketName, "acl", null);In this case, StringUtils.isEmpty(value) returns true, so the helper returns an empty ACL list without throwing. The PUT ?acl call succeeds with HTTP 200 and effectively behaves as “no grants provided”. @Test
public void testPutAclWithEmptyGrantHeaderValue() throws Exception {
assertEquals(200, resp.getStatus());
}
when(mockHeaders.getHeaderString(S3Acl.GRANT_FULL_CONTROL))
.thenReturn(" ");
Response resp = bucketEndpoint.put(bucketName, "acl", null);Here StringUtils.isEmpty(value) is false, so we attempt to parse the value. throw newError(S3ErrorTable.INVALID_ARGUMENT, acl);So this scenario fails with OS3Exception(INVALID_ARGUMENT, 400). @Test
public void testPutAclWithWhitespaceGrantHeaderValue() throws Exception {
OS3Exception ex = assertThrows(OS3Exception.class,
() -> bucketEndpoint.put(bucketName, "acl", null));
assertEquals(INVALID_ARGUMENT.getCode(), ex.getCode());
assertEquals(400, ex.getHttpCode());
}These two tests make the current behavior explicit and should help prevent regressions around header parsing for PUT ?acl. |
Gargi-jais11
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Russole for updating the patch.
Just one minor comment. Overall LGTM.
| .orElseThrow(() -> new AssertionError("owner-id ACL not found")); | ||
|
|
||
| assertEquals("owner-id", ownerAcl.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please also add an assert statement to verify the permission is READ (from header), not FULL_CONTROL (from body).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the requested assertions — the test now verifies that READ from the header is applied and FULL_CONTROL from the body is ignored.
|
@Russole there is no need to close the PR everytime you make changes. If you wish you can mark it as draft and after its done make it ready for review. |
Gargi-jais11
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patch LGTM!
|
Thank you for the clarification. |
|
Just checking in kindly. Since the patch already has LGTM, |
chungen0126
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Russole for the improvement. Left some comments.
|
|
||
| InputStream body = new ByteArrayInputStream(VALID_ACL_XML.getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| Response resp = bucketEndpoint.put(bucketName, "acl", body); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduce a static string constant to replace all hardcoded instances of 'acl'.
| InputStream body = new ByteArrayInputStream(VALID_ACL_XML.getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| Response resp = bucketEndpoint.put(bucketName, "acl", body); | ||
| assertEquals(200, resp.getStatus()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assertEquals(200, resp.getStatus()); | |
| assertEquals(HTTP_OK, resp.getStatus()); |
Use HTTP_OK to replace 200.
|
|
||
| Response resp = bucketEndpoint.put(bucketName, "acl", null); | ||
|
|
||
| assertEquals(200, resp.getStatus()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assertEquals(200, resp.getStatus()); | |
| assertEquals(HTTP_OK, resp.getStatus()); |
Use HTTP_OK to replace 200.
| VALID_ACL_XML.getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| Response resp = bucketEndpoint.put(bucketName, "acl", body); | ||
| assertEquals(200, resp.getStatus()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assertEquals(200, resp.getStatus()); | |
| assertEquals(HTTP_OK, resp.getStatus()); |
Use HTTP_OK to replace 200.
|
|
||
| Response resp = bucketEndpoint.put(bucketName, "acl", null); | ||
|
|
||
| Assertions.assertEquals(200, resp.getStatus()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Assertions.assertEquals(200, resp.getStatus()); | |
| assertEquals(HTTP_OK, resp.getStatus()); |
Use HTTP_OK to replace 200.
| OS3Exception os3 = (OS3Exception) cause; | ||
|
|
||
| assertEquals(INVALID_REQUEST.getCode(), os3.getCode()); | ||
| assertEquals(400, os3.getHttpCode()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assertEquals(400, os3.getHttpCode()); | |
| assertEquals(HTTP_BAD_REQUEST, os3.getHttpCode()); |
Use HTTP_BAD_REQUEST to replace 400.
| OS3Exception os3 = (OS3Exception) cause; | ||
|
|
||
| assertEquals(INVALID_REQUEST.getCode(), os3.getCode()); | ||
| assertEquals(400, os3.getHttpCode()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assertEquals(400, os3.getHttpCode()); | |
| assertEquals(HTTP_BAD_REQUEST, os3.getHttpCode()); |
Use HTTP_BAD_REQUEST to replace 400.
| () -> bucketEndpoint.put(bucketName, "acl", null)); | ||
|
|
||
| assertEquals(INVALID_ARGUMENT.getCode(), ex.getCode()); | ||
| assertEquals(400, ex.getHttpCode()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assertEquals(400, ex.getHttpCode()); | |
| assertEquals(HTTP_BAD_REQUEST, os3.getHttpCode()); |
Use HTTP_BAD_REQUEST to replace 400.
| () -> bucketEndpoint.put(bucketName, "acl", null)); | ||
|
|
||
| Assertions.assertEquals(INVALID_ARGUMENT.getCode(), ex.getCode()); | ||
| Assertions.assertEquals(400, ex.getHttpCode()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Assertions.assertEquals(400, ex.getHttpCode()); | |
| assertEquals(HTTP_BAD_REQUEST, os3.getHttpCode()); |
Use HTTP_BAD_REQUEST to replace 400.
| assertEquals(MALFORMED_HEADER.getCode(), ex.getCode()); | ||
| } | ||
| public void testAclWithMissingHeaders() throws Exception { | ||
| bucketEndpoint.getClient().getObjectStore().createS3Bucket(bucketName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to create an S3 bucket before bucket put?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because putAcl() first calls getS3Bucket(bucketName).
If the bucket does not exist, it throws NO_SUCH_BUCKET and never reaches any ACL parsing logic.
To test the ACL behavior, the bucket must already exist.
echonesis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Russole , left some comments.
| OS3Exception ex = assertThrows(OS3Exception.class, | ||
| () -> bucketEndpoint.put(bucketName, "acl", null)); | ||
|
|
||
| Assertions.assertEquals(INVALID_ARGUMENT.getCode(), ex.getCode()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Assertions.assertEquals(INVALID_ARGUMENT.getCode(), ex.getCode()); | |
| assertEquals(INVALID_ARGUMENT.getCode(), ex.getCode()); |
assertEquals is enough.
| bucketEndpoint.getClient().getObjectStore().createS3Bucket(bucketName); | ||
|
|
||
| when(mockHeaders.getHeaderString(S3Acl.GRANT_FULL_CONTROL)) | ||
| .thenReturn(" "); // whitespace only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider introducing a static constant for the whitespace string to improve readability:
| .thenReturn(" "); // whitespace only | |
| private static final String WHITESPACE_ONLY = " "; | |
| when(mockHeaders.getHeaderString(S3Acl.GRANT_FULL_CONTROL)) | |
| .thenReturn(WHITESPACE_ONLY); |
|
Thanks @echonesis and @chungen0126 for the review and helpful comments — really appreciate your guidance! 🙏 |
| " </Grant>" + | ||
| " </AccessControlList>" + | ||
| "</AccessControlPolicy>"; | ||
| private static final String ACL = "acl"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you could use OzoneConsts.ACL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion!
I've removed the local "acl" constant and updated the tests to use OzoneConsts.ACL so that we rely on the shared Ozone constant instead of redefining it.
|
Thanks to @Gargi-jais11, @chungen0126, and @echonesis for the review and helpful feedback. |
| () -> bucketEndpoint.put(bucketName, OzoneConsts.ACL, null)); | ||
|
|
||
| assertEquals(INVALID_ARGUMENT.getCode(), ex.getCode()); | ||
| assertEquals(400, ex.getHttpCode()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assertEquals(400, ex.getHttpCode()); | |
| assertEquals(HTTP_BAD_REQUEST, ex.getHttpCode()); |
Use HTTP_BAD_REQUEST to replace 400.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Russole for updating the patch. LGTM!
|
@ivandika3 and @peterxcli Please take a look on the patch. |
|
Thanks @Gargi-jais11 for the earlier ping. |
chungen0126
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Russole for the patch and sorry for the conflicts. Can you please move the test cases to TestBucketAcl or TestBucketAclHandler? BucketEndpoint.putAcl no longer exists (code refactored into BucketAclHandler). TestBucketAcl adds the ?acl query param, so you can use BucketEndpoint.put there. Please check if existing test cases cover any of the new ones.
| Response resp = bucketEndpoint.putAcl(bucketName, body); | ||
| assertEquals(HTTP_OK, resp.getStatus()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we have helper method to simplify this.
assertSucceeds(() -> bucketEndpoint.put(bucketName, body));| assertEquals(HTTP_NOT_FOUND, ex.getHttpCode()); | ||
| assertEquals(MALFORMED_HEADER.getCode(), ex.getCode()); | ||
| } | ||
| public void testAclWithMissingHeadersAndNoBody() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case ...MissingHeadersAndNoBody is no longer necessary.
| OS3Exception ex = assertThrows(OS3Exception.class, | ||
| () -> bucketEndpoint.putAcl(bucketName, null)); | ||
|
|
||
| assertEquals(INVALID_ARGUMENT.getCode(), ex.getCode()); | ||
| assertEquals(HTTP_BAD_REQUEST, ex.getHttpCode()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we have helper to simplify this:
assertErrorResponse(INVALID_ARGUMENT, () -> bucketEndpoint.put(bucketName, null));
What changes were proposed in this pull request?
This PR improves and expands the test coverage of the PUT Bucket ACL operation in BucketEndpoint, ensuring correct handling of S3 ACL semantics under different request formats.
Added VALID_ACL_XML constant to provide a reusable, well-formed S3 ACL body for tests.
Added new test cases to fully validate the behavior of PUT /{bucket}?acl:
Missing ACL headers with a valid XML body should succeed.
Missing ACL headers and missing body should fail with a WebApplicationException caused by an OS3Exception with InvalidRequest.
PUT ACL on a non-existing bucket should throw an OS3Exception with NoSuchBucket.
Header-based ACL update using x-amz-grant-full-control should succeed.
Invalid XML body (non-XML input) should trigger an OS3Exception with InvalidRequest.
Malformed ACL grant header (e.g., missing "=") should produce an OS3Exception with InvalidArgument.
Removed two ineffective test cases (testBucketFailWithAuthHeaderMissing and testBucketFailWithInvalidHeader) because they used a try/catch without assertThrows, causing them to pass even when no exception was thrown.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13932
How was this patch tested?
This patch includes new and enhanced unit tests in TestBucketPut to verify the updated behavior.
All tests were executed locally and passed.
Additionally, all CI pipelines passed after the patch was pushed.