smengcl commented on code in PR #3732:
URL: https://github.com/apache/ozone/pull/3732#discussion_r963156019
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneConfigUtil.java:
##########
@@ -30,6 +40,54 @@ public final class OzoneConfigUtil {
private OzoneConfigUtil() {
}
+ /**
+ * Return list of OzoneAdministrators from config.
+ */
+ static Collection<String> getOzoneAdminsFromConfig(OzoneConfiguration conf)
+ throws IOException {
+ Collection<String> ozAdmins =
+ conf.getTrimmedStringCollection(OZONE_ADMINISTRATORS);
+ String omSPN = UserGroupInformation.getCurrentUser().getShortUserName();
+ if (!ozAdmins.contains(omSPN)) {
+ ozAdmins.add(omSPN);
+ }
+ return ozAdmins;
+ }
+
+ /**
+ * Return list of s3 administrators prop from config.
Review Comment:
Let's put this in the javadoc as well:
```suggestion
* Return list of s3 administrators prop from config.
*
* If ozone.s3.administrators value is empty string or unset,
* defaults to ozone.administrators value.
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -329,11 +334,48 @@ public void testGetSecretOfAnotherUserAsNonAdmin() throws
IOException {
+ "the permission to get bob's secret in this test case!");
}
+ @Test
+ public void testGetSecretOfAnotherUserAsOzoneAdmin() throws IOException {
+ // This effectively makes alice an admin.
Review Comment:
```suggestion
// This effectively makes alice an S3 admin.
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -329,11 +334,48 @@ public void testGetSecretOfAnotherUserAsNonAdmin() throws
IOException {
+ "the permission to get bob's secret in this test case!");
}
+ @Test
+ public void testGetSecretOfAnotherUserAsOzoneAdmin() throws IOException {
+ // This effectively makes alice an admin.
+ when(ozoneManager.isS3Admin(ugiAlice)).thenReturn(true);
+
+
+ int txLogIndex = 1;
+
+ S3GetSecretRequest s3GetSecretRequest = new S3GetSecretRequest(
+ new S3GetSecretRequest(
+ s3GetSecretRequest(USER_CAROL)
+ ).preExecute(ozoneManager));
+
+ // Run validateAndUpdateCache
+ OMClientResponse omClientResponse =
+ s3GetSecretRequest.validateAndUpdateCache(ozoneManager,
+ txLogIndex, ozoneManagerDoubleBufferHelper);
+
+ // Check response type and cast
+ Assert.assertTrue(omClientResponse instanceof S3GetSecretResponse);
+ final S3GetSecretResponse s3GetSecretResponse =
+ (S3GetSecretResponse) omClientResponse;
+
+ // Check response
+ final S3SecretValue s3SecretValue = s3GetSecretResponse.getS3SecretValue();
+ Assert.assertEquals(USER_CAROL, s3SecretValue.getKerberosID());
+ final String awsSecret1 = s3SecretValue.getAwsSecret();
+ Assert.assertNotNull(awsSecret1);
+
+ final GetS3SecretResponse getS3SecretResponse =
+ s3GetSecretResponse.getOMResponse().getGetS3SecretResponse();
+ // The secret inside should be the same.
+ final S3Secret s3Secret1 = getS3SecretResponse.getS3Secret();
+ Assert.assertEquals(USER_CAROL, s3Secret1.getKerberosID());
+ Assert.assertEquals(awsSecret1, s3Secret1.getAwsSecret());
+ }
+
Review Comment:
Can we add a negative test case where e.g. `alice` is an Ozone admin, but
**not** an S3 admin, thus can't do S3 secret operations (rejected) for others?
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -196,7 +201,7 @@ private OMRequest s3GetSecretRequest(String
userPrincipalStr) {
public void testGetSecretOfAnotherUserAsAdmin() throws IOException {
// This effectively makes alice an admin.
Review Comment:
```suggestion
// This effectively makes alice an S3 admin.
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -329,11 +334,48 @@ public void testGetSecretOfAnotherUserAsNonAdmin() throws
IOException {
+ "the permission to get bob's secret in this test case!");
}
+ @Test
+ public void testGetSecretOfAnotherUserAsOzoneAdmin() throws IOException {
+ // This effectively makes alice an admin.
+ when(ozoneManager.isS3Admin(ugiAlice)).thenReturn(true);
+
+
+ int txLogIndex = 1;
+
+ S3GetSecretRequest s3GetSecretRequest = new S3GetSecretRequest(
+ new S3GetSecretRequest(
+ s3GetSecretRequest(USER_CAROL)
+ ).preExecute(ozoneManager));
+
+ // Run validateAndUpdateCache
+ OMClientResponse omClientResponse =
+ s3GetSecretRequest.validateAndUpdateCache(ozoneManager,
+ txLogIndex, ozoneManagerDoubleBufferHelper);
+
+ // Check response type and cast
+ Assert.assertTrue(omClientResponse instanceof S3GetSecretResponse);
+ final S3GetSecretResponse s3GetSecretResponse =
+ (S3GetSecretResponse) omClientResponse;
+
+ // Check response
+ final S3SecretValue s3SecretValue = s3GetSecretResponse.getS3SecretValue();
+ Assert.assertEquals(USER_CAROL, s3SecretValue.getKerberosID());
+ final String awsSecret1 = s3SecretValue.getAwsSecret();
+ Assert.assertNotNull(awsSecret1);
+
+ final GetS3SecretResponse getS3SecretResponse =
+ s3GetSecretResponse.getOMResponse().getGetS3SecretResponse();
+ // The secret inside should be the same.
+ final S3Secret s3Secret1 = getS3SecretResponse.getS3Secret();
+ Assert.assertEquals(USER_CAROL, s3Secret1.getKerberosID());
+ Assert.assertEquals(awsSecret1, s3Secret1.getAwsSecret());
+ }
+
@Test
public void testGetSecretWithTenant() throws IOException {
// This effectively makes alice an admin.
Review Comment:
```suggestion
// This effectively makes alice an S3 admin.
```
--
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]
