Skip to content

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.

Sign up for GitHub

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

Draft
Russole wants to merge 19 commits into
base: master
Choose a base branch
from
+188 −2
Draft

HDDS-13932. Add test cases for PUT bucket ACL #9371

Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
2d24393
HDDS-13932. Fix TestBucketPut by correcting test logic and updating t…
Russole Nov 24, 2025
1d5e9a1
Clean TestBucketPut Setup() Part
Russole Nov 25, 2025
ce0facd
Fix Unused Import
Russole Nov 25, 2025
230f88e
Ensure consistent usage of assertEquals() in TestBucketPut tests
Russole Nov 25, 2025
41b7a91
Use INVALID_REQUEST.getCode() instead of inline string InvalidRequest
Russole Nov 27, 2025
543f78a
Add testPutAclWithBothHeadersAndBody in TestBucketPut
Russole Nov 27, 2025
a20de05
Add testPutAclWithEmptyGrantHeaderValue and testPutAclWithWhitespaceG…
Russole Nov 27, 2025
e0b3ca9
Fix CheckStyle
Russole Nov 27, 2025
e899b94
Fix DeadStore
Russole Nov 27, 2025
bc3552b
Improve PUT ACL test coverage for header handling and precedence
Russole Dec 1, 2025
32735b5
Merge branch 'master' into HDDS-13932
Russole Dec 8, 2025
5257527
Refine implementation based on review comments
Russole Dec 8, 2025
58075a2
Replace literal 400 with HTTP_BAD_REQUEST
Russole Dec 8, 2025
3de3aac
Replace literal acl with OzoneConsts.ACL in TestBucketPut
Russole Dec 9, 2025
c426daa
Replace 400 with HTTP_BAD_REQUEST
Russole Dec 15, 2025
99dc3d8
Merge branch 'master' into HDDS-13932
Russole Dec 23, 2025
8992171
Update Put ACL tests to align with refactored endpoint
Russole Dec 23, 2025
331719d
Fix CheckStyle
Russole Dec 23, 2025
ee29e8d
Merge branch 'apache:master' into HDDS-13932
Russole Jan 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
190 changes: 188 additions & 2 deletions hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketPut.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,38 @@

package org.apache.hadoop.ozone.s3.endpoint;

import static java.net.HttpURLConnection.HTTP_BAD_REQUEST;
import static java.net.HttpURLConnection.HTTP_CONFLICT;
import static java.net.HttpURLConnection.HTTP_NOT_FOUND;
import static java.net.HttpURLConnection.HTTP_OK;
import static org.apache.hadoop.ozone.OzoneAcl.AclScope.ACCESS;
import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.BUCKET_ALREADY_EXISTS;
import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.INVALID_ARGUMENT;
import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.INVALID_REQUEST;
import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.MALFORMED_HEADER;
import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.NO_SUCH_BUCKET;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.io.ByteArrayInputStream;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.util.List;
import javax.ws.rs.WebApplicationException;
import javax.ws.rs.core.HttpHeaders;
import javax.ws.rs.core.Response;
import org.apache.hadoop.ozone.OzoneAcl;
import org.apache.hadoop.ozone.OzoneConsts;
import org.apache.hadoop.ozone.client.OzoneBucket;
import org.apache.hadoop.ozone.client.OzoneClient;
import org.apache.hadoop.ozone.client.OzoneClientStub;
import org.apache.hadoop.ozone.s3.exception.OS3Exception;
import org.apache.hadoop.ozone.security.acl.IAccessAuthorizer;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

Expand All @@ -40,6 +59,25 @@ public class TestBucketPut {

private String bucketName = OzoneConsts.BUCKET;
private BucketEndpoint bucketEndpoint;
private HttpHeaders mockHeaders;
private static final String VALID_ACL_XML =
"<AccessControlPolicy xmlns=\"http://s3.amazonaws.com/doc/2006-03-01/\" " +
" xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\">" +
" <Owner>" +
" <ID>owner-id</ID>" +
" <DisplayName>owner-name</DisplayName>" +
" </Owner>" +
" <AccessControlList>" +
" <Grant>" +
" <Grantee xsi:type=\"CanonicalUser\">" +
" <ID>owner-id</ID>" +
" <DisplayName>owner-name</DisplayName>" +
" </Grantee>" +
" <Permission>FULL_CONTROL</Permission>" +
" </Grant>" +
" </AccessControlList>" +
"</AccessControlPolicy>";
private static final String WHITESPACE_ONLY = " ";

@BeforeEach
public void setup() throws Exception {
Expand All @@ -51,6 +89,36 @@ public void setup() throws Exception {
bucketEndpoint = EndpointBuilder.newBucketEndpointBuilder()
.setClient(clientStub)
.build();

mockHeaders = mock(HttpHeaders.class);
bucketEndpoint.setHeaders(mockHeaders);
}

@Test
public void testAclWithMissingHeaders() throws Exception {
bucketEndpoint.getClient().getObjectStore().createS3Bucket(bucketName);
Copy link
Contributor

@chungen0126 chungen0126 Dec 8, 2025

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?

Copy link
Contributor Author

@Russole Russole Dec 8, 2025

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.


InputStream body = new ByteArrayInputStream(VALID_ACL_XML.getBytes(StandardCharsets.UTF_8));

Response resp = bucketEndpoint.putAcl(bucketName, body);
assertEquals(HTTP_OK, resp.getStatus());
Comment on lines +103 to +104
Copy link
Contributor

@adoroszlai adoroszlai Jan 15, 2026

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));

}

@Test
public void testAclWithMissingHeadersAndNoBody() throws Exception {
Copy link
Contributor

@Gargi-jais11 Gargi-jais11 Nov 26, 2025

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)

Copy link
Contributor Author

@Russole Russole Nov 28, 2025

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.

Copy link
Contributor

@adoroszlai adoroszlai Jan 15, 2026

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.

bucketEndpoint.getClient().getObjectStore().createS3Bucket(bucketName);

WebApplicationException wae = assertThrows(WebApplicationException.class,
() -> bucketEndpoint.putAcl(bucketName, null));

Throwable cause = wae.getCause();
assertNotNull(cause);
assertTrue(cause instanceof OS3Exception);

OS3Exception os3 = (OS3Exception) cause;

assertEquals(INVALID_REQUEST.getCode(), os3.getCode());
assertEquals(HTTP_BAD_REQUEST, os3.getHttpCode());
}

@Test
Expand All @@ -66,14 +134,106 @@ public void testBucketFailWithAuthHeaderMissing() throws Exception {
@Test
public void testBucketPut() throws Exception {
Response response = bucketEndpoint.put(bucketName, null);
assertEquals(200, response.getStatus());
assertEquals(HTTP_OK, response.getStatus());
assertNotNull(response.getLocation());

// Create-bucket on an existing bucket fails
OS3Exception e = assertThrows(OS3Exception.class, () -> bucketEndpoint.put(
bucketName, null));
assertEquals(HTTP_CONFLICT, e.getHttpCode());
assertEquals(BUCKET_ALREADY_EXISTS.getCode(), e.getCode());
assertEquals(HTTP_CONFLICT, e.getHttpCode());
}

@Test
public void testPutAclOnNonExistingBucket() throws Exception {
OS3Exception e = assertThrows(OS3Exception.class,
() -> bucketEndpoint.putAcl(bucketName, null));
assertEquals(NO_SUCH_BUCKET.getCode(), e.getCode());
assertEquals(HTTP_NOT_FOUND, e.getHttpCode());
}

@Test
public void testPutAclWithGrantFullControlHeader() throws Exception {
bucketEndpoint.getClient().getObjectStore().createS3Bucket(bucketName);

when(mockHeaders.getHeaderString(S3Acl.GRANT_FULL_CONTROL))
.thenReturn("id=\"owner-id\"");

Response resp = bucketEndpoint.putAcl(bucketName, null);

assertEquals(HTTP_OK, resp.getStatus());
}

@Test
public void testPutAclWithInvalidXmlBody() throws Exception {
bucketEndpoint.getClient().getObjectStore().createS3Bucket(bucketName);

InputStream body = new ByteArrayInputStream(
"not-xml".getBytes(StandardCharsets.UTF_8));

WebApplicationException wae = assertThrows(WebApplicationException.class,
() -> bucketEndpoint.putAcl(bucketName, body));

Throwable cause = wae.getCause();
assertNotNull(cause);
assertTrue(cause instanceof OS3Exception);
OS3Exception os3 = (OS3Exception) cause;

assertEquals(INVALID_REQUEST.getCode(), os3.getCode());
assertEquals(HTTP_BAD_REQUEST, os3.getHttpCode());
}

@Test
public void testPutAclWithMalformedGrantHeader() throws Exception {
bucketEndpoint.getClient().getObjectStore().createS3Bucket(bucketName);

when(mockHeaders.getHeaderString(S3Acl.GRANT_FULL_CONTROL))
.thenReturn("id\"owner-id\"");

OS3Exception os3 = assertThrows(OS3Exception.class,
() -> bucketEndpoint.putAcl(bucketName, null));

assertEquals(INVALID_ARGUMENT.getCode(), os3.getCode());
assertEquals(HTTP_BAD_REQUEST, os3.getHttpCode());
}

@Test
public void testPutAclWithBothHeadersAndBody() throws Exception {
bucketEndpoint.getClient().getObjectStore().createS3Bucket(bucketName);

// Header: READ
when(mockHeaders.getHeaderString(S3Acl.GRANT_READ))
.thenReturn("id=owner-id");

// Body: FULL_CONTROL
InputStream body = new ByteArrayInputStream(
VALID_ACL_XML.getBytes(StandardCharsets.UTF_8));

Response resp = bucketEndpoint.putAcl(bucketName, body);
assertEquals(HTTP_OK, resp.getStatus());

OzoneBucket bucket = bucketEndpoint.getClient()
.getObjectStore()
.getS3Bucket(bucketName);

List<OzoneAcl> acls = bucket.getAcls();
assertFalse(acls.isEmpty());

OzoneAcl ownerAcl = acls.stream()
.filter(acl -> "owner-id".equals(acl.getName())
&& acl.getAclScope() == ACCESS)
.findFirst()
.orElseThrow(() -> new AssertionError("owner-id ACL not found"));

assertEquals("owner-id", ownerAcl.getName());
Comment on lines +226 to +228
Copy link
Contributor

@Gargi-jais11 Gargi-jais11 Dec 1, 2025

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).

Copy link
Contributor Author

@Russole Russole Dec 1, 2025

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.


List<IAccessAuthorizer.ACLType> permissions = ownerAcl.getAclList();

assertTrue(permissions.contains(IAccessAuthorizer.ACLType.READ),
"Expected READ permission from header");

assertFalse(permissions.contains(IAccessAuthorizer.ACLType.ALL),
"FULL_CONTROL/ALL from body should not be applied when header is present");
}

@Test
Expand All @@ -85,4 +245,30 @@ public void testBucketFailWithInvalidHeader() throws Exception {
assertEquals(MALFORMED_HEADER.getCode(), ex.getCode());
}
}

@Test
public void testPutAclWithEmptyGrantHeaderValue() throws Exception {
bucketEndpoint.getClient().getObjectStore().createS3Bucket(bucketName);

when(mockHeaders.getHeaderString(S3Acl.GRANT_FULL_CONTROL))
.thenReturn(""); // empty

Response resp = bucketEndpoint.putAcl(bucketName, null);

assertEquals(HTTP_OK, resp.getStatus());
}

@Test
public void testPutAclWithWhitespaceGrantHeaderValue() throws Exception {
bucketEndpoint.getClient().getObjectStore().createS3Bucket(bucketName);

when(mockHeaders.getHeaderString(S3Acl.GRANT_FULL_CONTROL))
.thenReturn(WHITESPACE_ONLY); // whitespace only

OS3Exception ex = assertThrows(OS3Exception.class,
() -> bucketEndpoint.putAcl(bucketName, null));

assertEquals(INVALID_ARGUMENT.getCode(), ex.getCode());
assertEquals(HTTP_BAD_REQUEST, ex.getHttpCode());
Comment on lines +268 to +272
Copy link
Contributor

@adoroszlai adoroszlai Jan 15, 2026

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));

}
}