This is an automated email from the ASF dual-hosted git repository.
jshao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 5b81a1da3 [#3232] fix(client): Fix the deleting role issues with
irregular name (#4501)
5b81a1da3 is described below
commit 5b81a1da3e070b03f0366a800fecaa045a55d221
Author: roryqi <[email protected]>
AuthorDate: Wed Aug 14 23:17:00 2024 +0800
[#3232] fix(client): Fix the deleting role issues with irregular name
(#4501)
### What changes were proposed in this pull request?
Fix the deleting role issues with irregular name.
Add the e2e IT for access control
Fix the issue of null name issue about securable object in the client.
Use NoSuchMetadataObjectException instead of IlegallException for not
existed securable object.
### Why are the changes needed?
Fix: #3232
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Add some ITs
---
.../org/apache/gravitino/client/ErrorHandlers.java | 4 +
.../apache/gravitino/client/GravitinoClient.java | 3 +-
.../apache/gravitino/client/GravitinoMetalake.java | 42 +++-
.../dto/authorization/SecurableObjectDTO.java | 45 ++--
.../test/authorization/AccessControlIT.java | 270 +++++++++++++++++++++
.../{OwnerPostHookIT.java => OwnerIT.java} | 87 ++++++-
.../gravitino/server/web/rest/RoleOperations.java | 15 +-
.../server/web/rest/TestRoleOperations.java | 15 +-
8 files changed, 427 insertions(+), 54 deletions(-)
diff --git
a/clients/client-java/src/main/java/org/apache/gravitino/client/ErrorHandlers.java
b/clients/client-java/src/main/java/org/apache/gravitino/client/ErrorHandlers.java
index 4741e8462..18576e277 100644
---
a/clients/client-java/src/main/java/org/apache/gravitino/client/ErrorHandlers.java
+++
b/clients/client-java/src/main/java/org/apache/gravitino/client/ErrorHandlers.java
@@ -627,6 +627,10 @@ public class ErrorHandlers {
throw new NoSuchMetalakeException(errorMessage);
} else if
(errorResponse.getType().equals(NoSuchRoleException.class.getSimpleName())) {
throw new NoSuchRoleException(errorMessage);
+ } else if (errorResponse
+ .getType()
+ .equals(NoSuchMetadataObjectException.class.getSimpleName())) {
+ throw new NoSuchMetadataObjectException(errorMessage);
} else {
throw new NotFoundException(errorMessage);
}
diff --git
a/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoClient.java
b/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoClient.java
index 962bb09e1..9b7769200 100644
---
a/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoClient.java
+++
b/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoClient.java
@@ -242,11 +242,12 @@ public class GravitinoClient extends GravitinoClientBase
* @return The created Role instance.
* @throws RoleAlreadyExistsException If a Role with the same name already
exists.
* @throws NoSuchMetalakeException If the Metalake with the given name does
not exist.
+ * @throws NoSuchMetadataObjectException If securable object doesn't exist
* @throws RuntimeException If creating the Role encounters storage issues.
*/
public Role createRole(
String role, Map<String, String> properties, List<SecurableObject>
securableObjects)
- throws RoleAlreadyExistsException, NoSuchMetalakeException {
+ throws RoleAlreadyExistsException, NoSuchMetalakeException,
NoSuchMetadataObjectException {
return getMetalake().createRole(role, properties, securableObjects);
}
/**
diff --git
a/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java
b/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java
index 08685e1a2..f13958cb5 100644
---
a/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java
+++
b/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java
@@ -82,6 +82,7 @@ import org.apache.gravitino.exceptions.NotFoundException;
import org.apache.gravitino.exceptions.RoleAlreadyExistsException;
import org.apache.gravitino.exceptions.TagAlreadyExistsException;
import org.apache.gravitino.exceptions.UserAlreadyExistsException;
+import org.apache.gravitino.rest.RESTUtils;
import org.apache.gravitino.tag.Tag;
import org.apache.gravitino.tag.TagChange;
import org.apache.gravitino.tag.TagOperations;
@@ -484,7 +485,7 @@ public class GravitinoMetalake extends MetalakeDTO
implements SupportsCatalogs,
public boolean removeUser(String user) throws NoSuchMetalakeException {
RemoveResponse resp =
restClient.delete(
- String.format(API_METALAKES_USERS_PATH, this.name(), user),
+ String.format(API_METALAKES_USERS_PATH, this.name(),
RESTUtils.encodeString(user)),
RemoveResponse.class,
Collections.emptyMap(),
ErrorHandlers.userErrorHandler());
@@ -505,7 +506,7 @@ public class GravitinoMetalake extends MetalakeDTO
implements SupportsCatalogs,
public User getUser(String user) throws NoSuchUserException,
NoSuchMetalakeException {
UserResponse resp =
restClient.get(
- String.format(API_METALAKES_USERS_PATH, this.name(), user),
+ String.format(API_METALAKES_USERS_PATH, this.name(),
RESTUtils.encodeString(user)),
UserResponse.class,
Collections.emptyMap(),
ErrorHandlers.userErrorHandler());
@@ -551,7 +552,7 @@ public class GravitinoMetalake extends MetalakeDTO
implements SupportsCatalogs,
public boolean removeGroup(String group) throws NoSuchMetalakeException {
RemoveResponse resp =
restClient.delete(
- String.format(API_METALAKES_GROUPS_PATH, this.name(), group),
+ String.format(API_METALAKES_GROUPS_PATH, this.name(),
RESTUtils.encodeString(group)),
RemoveResponse.class,
Collections.emptyMap(),
ErrorHandlers.groupErrorHandler());
@@ -572,7 +573,7 @@ public class GravitinoMetalake extends MetalakeDTO
implements SupportsCatalogs,
public Group getGroup(String group) throws NoSuchGroupException,
NoSuchMetalakeException {
GroupResponse resp =
restClient.get(
- String.format(API_METALAKES_GROUPS_PATH, this.name(), group),
+ String.format(API_METALAKES_GROUPS_PATH, this.name(),
RESTUtils.encodeString(group)),
GroupResponse.class,
Collections.emptyMap(),
ErrorHandlers.groupErrorHandler());
@@ -593,7 +594,7 @@ public class GravitinoMetalake extends MetalakeDTO
implements SupportsCatalogs,
public Role getRole(String role) throws NoSuchRoleException,
NoSuchMetalakeException {
RoleResponse resp =
restClient.get(
- String.format(API_METALAKES_ROLES_PATH, this.name(), role),
+ String.format(API_METALAKES_ROLES_PATH, this.name(),
RESTUtils.encodeString(role)),
RoleResponse.class,
Collections.emptyMap(),
ErrorHandlers.roleErrorHandler());
@@ -614,7 +615,7 @@ public class GravitinoMetalake extends MetalakeDTO
implements SupportsCatalogs,
public boolean deleteRole(String role) throws NoSuchMetalakeException {
DeleteResponse resp =
restClient.delete(
- String.format(API_METALAKES_ROLES_PATH, this.name(), role),
+ String.format(API_METALAKES_ROLES_PATH, this.name(),
RESTUtils.encodeString(role)),
DeleteResponse.class,
Collections.emptyMap(),
ErrorHandlers.roleErrorHandler());
@@ -632,11 +633,12 @@ public class GravitinoMetalake extends MetalakeDTO
implements SupportsCatalogs,
* @return The created Role instance.
* @throws RoleAlreadyExistsException If a Role with the same name already
exists.
* @throws NoSuchMetalakeException If the Metalake with the given name does
not exist.
+ * @throws NoSuchMetadataObjectException If the securable object doesn't
exist
* @throws RuntimeException If creating the Role encounters storage issues.
*/
public Role createRole(
String role, Map<String, String> properties, List<SecurableObject>
securableObjects)
- throws RoleAlreadyExistsException, NoSuchMetalakeException {
+ throws RoleAlreadyExistsException, NoSuchMetalakeException,
NoSuchMetadataObjectException {
RoleCreateRequest req =
new RoleCreateRequest(
role,
@@ -676,7 +678,10 @@ public class GravitinoMetalake extends MetalakeDTO
implements SupportsCatalogs,
UserResponse resp =
restClient.put(
- String.format(API_PERMISSION_PATH, this.name(),
String.format("users/%s/grant", user)),
+ String.format(
+ API_PERMISSION_PATH,
+ this.name(),
+ String.format("users/%s/grant", RESTUtils.encodeString(user))),
request,
UserResponse.class,
Collections.emptyMap(),
@@ -705,7 +710,9 @@ public class GravitinoMetalake extends MetalakeDTO
implements SupportsCatalogs,
GroupResponse resp =
restClient.put(
String.format(
- API_PERMISSION_PATH, this.name(),
String.format("groups/%s/grant", group)),
+ API_PERMISSION_PATH,
+ this.name(),
+ String.format("groups/%s/grant",
RESTUtils.encodeString(group))),
request,
GroupResponse.class,
Collections.emptyMap(),
@@ -733,7 +740,10 @@ public class GravitinoMetalake extends MetalakeDTO
implements SupportsCatalogs,
UserResponse resp =
restClient.put(
- String.format(API_PERMISSION_PATH, this.name(),
String.format("users/%s/revoke", user)),
+ String.format(
+ API_PERMISSION_PATH,
+ this.name(),
+ String.format("users/%s/revoke",
RESTUtils.encodeString(user))),
request,
UserResponse.class,
Collections.emptyMap(),
@@ -762,7 +772,9 @@ public class GravitinoMetalake extends MetalakeDTO
implements SupportsCatalogs,
GroupResponse resp =
restClient.put(
String.format(
- API_PERMISSION_PATH, this.name(),
String.format("groups/%s/revoke", group)),
+ API_PERMISSION_PATH,
+ this.name(),
+ String.format("groups/%s/revoke",
RESTUtils.encodeString(group))),
request,
GroupResponse.class,
Collections.emptyMap(),
@@ -787,7 +799,9 @@ public class GravitinoMetalake extends MetalakeDTO
implements SupportsCatalogs,
API_METALAKES_OWNERS_PATH,
this.name(),
String.format(
- "%s/%s", object.type().name().toLowerCase(Locale.ROOT),
object.fullName())),
+ "%s/%s",
+ object.type().name().toLowerCase(Locale.ROOT),
+ RESTUtils.encodeString(object.fullName()))),
OwnerResponse.class,
Collections.emptyMap(),
ErrorHandlers.ownerErrorHandler());
@@ -813,7 +827,9 @@ public class GravitinoMetalake extends MetalakeDTO
implements SupportsCatalogs,
API_METALAKES_OWNERS_PATH,
this.name(),
String.format(
- "%s/%s", object.type().name().toLowerCase(Locale.ROOT),
object.fullName())),
+ "%s/%s",
+ object.type().name().toLowerCase(Locale.ROOT),
+ RESTUtils.encodeString(object.fullName()))),
request,
SetResponse.class,
Collections.emptyMap(),
diff --git
a/common/src/main/java/org/apache/gravitino/dto/authorization/SecurableObjectDTO.java
b/common/src/main/java/org/apache/gravitino/dto/authorization/SecurableObjectDTO.java
index 8a10d93df..710147994 100644
---
a/common/src/main/java/org/apache/gravitino/dto/authorization/SecurableObjectDTO.java
+++
b/common/src/main/java/org/apache/gravitino/dto/authorization/SecurableObjectDTO.java
@@ -31,9 +31,6 @@ import org.apache.gravitino.authorization.SecurableObject;
/** Data transfer object representing a securable object. */
public class SecurableObjectDTO implements SecurableObject {
- @JsonProperty("fullName")
- private String fullName;
-
@JsonProperty("type")
private Type type;
@@ -46,27 +43,27 @@ public class SecurableObjectDTO implements SecurableObject {
/** Default constructor for Jackson deserialization. */
protected SecurableObjectDTO() {}
+ /** @return The full name of the securable object. */
+ @JsonProperty("fullName")
+ public String getFullName() {
+ return fullName();
+ }
+
/**
- * Creates a new instance of SecurableObject DTO.
+ * Sets the full name of the securable object. Only used by Jackson
deserializer.
*
- * @param privileges The privileges of the SecurableObject DTO.
- * @param fullName The name of the SecurableObject DTO.
- * @param type The type of the securable object.
+ * @param fullName The full name of the metadata object.
*/
- protected SecurableObjectDTO(String fullName, Type type, PrivilegeDTO[]
privileges) {
- this.type = type;
- this.fullName = fullName;
+ @JsonProperty("fullName")
+ public void setFullName(String fullName) {
int index = fullName.lastIndexOf(".");
-
if (index == -1) {
- this.parent = null;
- this.name = fullName;
+ parent = null;
+ name = fullName;
} else {
- this.parent = fullName.substring(0, index);
- this.name = fullName.substring(index + 1);
+ parent = fullName.substring(0, index);
+ name = fullName.substring(index + 1);
}
-
- this.privileges = privileges;
}
@Nullable
@@ -80,11 +77,6 @@ public class SecurableObjectDTO implements SecurableObject {
return name;
}
- @Override
- public String fullName() {
- return fullName;
- }
-
@Override
public Type type() {
return type;
@@ -106,10 +98,13 @@ public class SecurableObjectDTO implements SecurableObject
{
/** Builder for {@link SecurableObjectDTO}. */
public static class Builder {
+ private final SecurableObjectDTO securableObjectDTO = new
SecurableObjectDTO();
private String fullName;
private Type type;
private PrivilegeDTO[] privileges;
+ private Builder() {}
+
/**
* Sets the full name of the securable object.
*
@@ -158,9 +153,11 @@ public class SecurableObjectDTO implements SecurableObject
{
Preconditions.checkArgument(
privileges != null && privileges.length != 0, "privileges can't be
null or empty");
- SecurableObjectDTO object = new SecurableObjectDTO(fullName, type,
privileges);
+ securableObjectDTO.type = type;
+ securableObjectDTO.privileges = privileges;
+ securableObjectDTO.setFullName(fullName);
- return object;
+ return securableObjectDTO;
}
}
}
diff --git
a/integration-test/src/test/java/org/apache/gravitino/integration/test/authorization/AccessControlIT.java
b/integration-test/src/test/java/org/apache/gravitino/integration/test/authorization/AccessControlIT.java
new file mode 100644
index 000000000..62366d075
--- /dev/null
+++
b/integration-test/src/test/java/org/apache/gravitino/integration/test/authorization/AccessControlIT.java
@@ -0,0 +1,270 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.integration.test.authorization;
+
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+import java.util.Collections;
+import java.util.Map;
+import org.apache.gravitino.Configs;
+import org.apache.gravitino.auth.AuthConstants;
+import org.apache.gravitino.authorization.Group;
+import org.apache.gravitino.authorization.Privilege;
+import org.apache.gravitino.authorization.Privileges;
+import org.apache.gravitino.authorization.Role;
+import org.apache.gravitino.authorization.SecurableObject;
+import org.apache.gravitino.authorization.SecurableObjects;
+import org.apache.gravitino.authorization.User;
+import org.apache.gravitino.client.GravitinoMetalake;
+import org.apache.gravitino.exceptions.GroupAlreadyExistsException;
+import org.apache.gravitino.exceptions.NoSuchGroupException;
+import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
+import org.apache.gravitino.exceptions.NoSuchRoleException;
+import org.apache.gravitino.exceptions.NoSuchUserException;
+import org.apache.gravitino.exceptions.UserAlreadyExistsException;
+import org.apache.gravitino.integration.test.util.AbstractIT;
+import org.apache.gravitino.utils.RandomNameUtils;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+public class AccessControlIT extends AbstractIT {
+
+ private static String metalakeName =
RandomNameUtils.genRandomName("metalake");
+ private static GravitinoMetalake metalake;
+
+ @BeforeAll
+ public static void startIntegrationTest() throws Exception {
+ Map<String, String> configs = Maps.newHashMap();
+ configs.put(Configs.ENABLE_AUTHORIZATION.getKey(), String.valueOf(true));
+ configs.put(Configs.SERVICE_ADMINS.getKey(), AuthConstants.ANONYMOUS_USER);
+ registerCustomConfigs(configs);
+ AbstractIT.startIntegrationTest();
+ metalake = client.createMetalake(metalakeName, "metalake comment",
Collections.emptyMap());
+ }
+
+ @Test
+ void testManageUsers() {
+ String username = "user1#123";
+ User user = metalake.addUser(username);
+ Assertions.assertEquals(username, user.name());
+ Assertions.assertTrue(user.roles().isEmpty());
+
+ // Add an existed user
+ Assertions.assertThrows(UserAlreadyExistsException.class, () ->
metalake.addUser(username));
+
+ user = metalake.getUser(username);
+ Assertions.assertEquals(username, user.name());
+ Assertions.assertTrue(user.roles().isEmpty());
+
+ // Get a not-existed user
+ Assertions.assertThrows(NoSuchUserException.class, () ->
metalake.getUser("not-existed"));
+
+ Assertions.assertTrue(metalake.removeUser(username));
+ Assertions.assertFalse(metalake.removeUser(username));
+ }
+
+ @Test
+ void testManageGroups() {
+ String groupName = "group1#123";
+ Group group = metalake.addGroup(groupName);
+ Assertions.assertEquals(group.name(), groupName);
+ Assertions.assertTrue(group.roles().isEmpty());
+
+ // Added an existed group
+ Assertions.assertThrows(GroupAlreadyExistsException.class, () ->
metalake.addGroup(groupName));
+
+ group = metalake.getGroup(groupName);
+ Assertions.assertEquals(group.name(), groupName);
+ Assertions.assertTrue(group.roles().isEmpty());
+
+ // Get a not-existed group
+ Assertions.assertThrows(NoSuchGroupException.class, () ->
metalake.getGroup("not-existed"));
+
+ Assertions.assertTrue(metalake.removeGroup(groupName));
+ Assertions.assertFalse(metalake.removeGroup(groupName));
+ }
+
+ @Test
+ void testManageRoles() {
+ String roleName = "role#123";
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put("k1", "v1");
+ SecurableObject metalakeObject =
+ SecurableObjects.ofMetalake(
+ metalakeName,
Lists.newArrayList(Privileges.CreateCatalog.allow()));
+ Role role = metalake.createRole(roleName, properties,
Lists.newArrayList(metalakeObject));
+
+ Assertions.assertEquals(roleName, role.name());
+ Assertions.assertEquals(properties, role.properties());
+
+ // Verify the object
+ Assertions.assertEquals(1, role.securableObjects().size());
+ SecurableObject createdObject = role.securableObjects().get(0);
+ Assertions.assertEquals(metalakeObject.name(), createdObject.name());
+ Assertions.assertEquals(metalakeObject.type(), createdObject.type());
+
+ // Verify the privilege
+ Assertions.assertEquals(1, createdObject.privileges().size());
+ Privilege createdPrivilege = createdObject.privileges().get(0);
+ Assertions.assertEquals(createdPrivilege.name(),
Privilege.Name.CREATE_CATALOG);
+ Assertions.assertEquals(createdPrivilege.condition(),
Privilege.Condition.ALLOW);
+
+ // Test a not-existed metadata object
+ SecurableObject catalogObject =
+ SecurableObjects.ofCatalog(
+ "not-existed", Lists.newArrayList(Privileges.UseCatalog.allow()));
+
+ Assertions.assertThrows(
+ NoSuchMetadataObjectException.class,
+ () -> metalake.createRole("not-existed", properties,
Lists.newArrayList(catalogObject)));
+
+ // Get a role
+ role = metalake.getRole(roleName);
+
+ Assertions.assertEquals(roleName, role.name());
+ Assertions.assertEquals(properties, role.properties());
+
+ // Verify the object
+ Assertions.assertEquals(1, role.securableObjects().size());
+ createdObject = role.securableObjects().get(0);
+ Assertions.assertEquals(metalakeObject.name(), createdObject.name());
+ Assertions.assertEquals(metalakeObject.type(), createdObject.type());
+
+ // Verify the privilege
+ Assertions.assertEquals(1, createdObject.privileges().size());
+ createdPrivilege = createdObject.privileges().get(0);
+ Assertions.assertEquals(createdPrivilege.name(),
Privilege.Name.CREATE_CATALOG);
+ Assertions.assertEquals(createdPrivilege.condition(),
Privilege.Condition.ALLOW);
+
+ // Delete a role
+ Assertions.assertTrue(metalake.deleteRole(roleName));
+ Assertions.assertFalse(metalake.deleteRole(roleName));
+ }
+
+ @Test
+ void testManageUserPermissions() {
+ String username = "user1#123";
+ User user = metalake.addUser(username);
+ Assertions.assertTrue(user.roles().isEmpty());
+
+ String roleName = "role#123";
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put("k1", "v1");
+ SecurableObject metalakeObject =
+ SecurableObjects.ofMetalake(
+ metalakeName,
Lists.newArrayList(Privileges.CreateCatalog.allow()));
+ metalake.createRole(roleName, properties,
Lists.newArrayList(metalakeObject));
+
+ user = metalake.grantRolesToUser(Lists.newArrayList(roleName), username);
+ Assertions.assertEquals(Lists.newArrayList(roleName), user.roles());
+
+ user = metalake.revokeRolesFromUser(Lists.newArrayList(roleName),
username);
+ Assertions.assertTrue(user.roles().isEmpty());
+
+ // Grant a not-existed role
+ Assertions.assertThrows(
+ NoSuchRoleException.class,
+ () -> metalake.grantRolesToUser(Lists.newArrayList("not-existed"),
username));
+
+ // Revoke a not-existed role
+ Assertions.assertThrows(
+ NoSuchRoleException.class,
+ () -> metalake.revokeRolesFromUser(Lists.newArrayList("not-existed"),
username));
+
+ // Grant to a not-existed user
+ Assertions.assertThrows(
+ NoSuchUserException.class,
+ () -> metalake.grantRolesToUser(Lists.newArrayList(roleName),
"not-existed"));
+
+ // Revoke from a not-existed user
+ Assertions.assertThrows(
+ NoSuchUserException.class,
+ () -> metalake.grantRolesToUser(Lists.newArrayList(roleName),
"not-existed"));
+
+ // Grant a granted role
+ metalake.grantRolesToUser(Lists.newArrayList(roleName), username);
+ Assertions.assertDoesNotThrow(
+ () -> metalake.grantRolesToUser(Lists.newArrayList(roleName),
username));
+
+ // Revoke a revoked role
+ metalake.revokeRolesFromUser(Lists.newArrayList(roleName), username);
+ Assertions.assertDoesNotThrow(
+ () -> metalake.revokeRolesFromUser(Lists.newArrayList(roleName),
username));
+
+ // Clean up
+ metalake.removeUser(username);
+ metalake.deleteRole(roleName);
+ }
+
+ @Test
+ void testManageGroupPermissions() {
+ String groupName = "group1#123";
+ Group group = metalake.addGroup(groupName);
+ Assertions.assertTrue(group.roles().isEmpty());
+
+ String roleName = "role#123";
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put("k1", "v1");
+ SecurableObject metalakeObject =
+ SecurableObjects.ofMetalake(
+ metalakeName,
Lists.newArrayList(Privileges.CreateCatalog.allow()));
+ metalake.createRole(roleName, properties,
Lists.newArrayList(metalakeObject));
+
+ group = metalake.grantRolesToGroup(Lists.newArrayList(roleName),
groupName);
+ Assertions.assertEquals(Lists.newArrayList(roleName), group.roles());
+
+ group = metalake.revokeRolesFromGroup(Lists.newArrayList(roleName),
groupName);
+ Assertions.assertTrue(group.roles().isEmpty());
+
+ // Grant a not-existed role
+ Assertions.assertThrows(
+ NoSuchRoleException.class,
+ () -> metalake.grantRolesToGroup(Lists.newArrayList("not-existed"),
groupName));
+
+ // Revoke a not-existed role
+ Assertions.assertThrows(
+ NoSuchRoleException.class,
+ () -> metalake.revokeRolesFromGroup(Lists.newArrayList("not-existed"),
groupName));
+
+ // Grant to a not-existed group
+ Assertions.assertThrows(
+ NoSuchGroupException.class,
+ () -> metalake.grantRolesToGroup(Lists.newArrayList(roleName),
"not-existed"));
+
+ // Revoke from a not-existed group
+ Assertions.assertThrows(
+ NoSuchGroupException.class,
+ () -> metalake.grantRolesToGroup(Lists.newArrayList(roleName),
"not-existed"));
+
+ // Grant a granted role
+ metalake.grantRolesToGroup(Lists.newArrayList(roleName), groupName);
+ Assertions.assertDoesNotThrow(
+ () -> metalake.grantRolesToGroup(Lists.newArrayList(roleName),
groupName));
+
+ // Revoke a revoked role
+ metalake.revokeRolesFromGroup(Lists.newArrayList(roleName), groupName);
+ Assertions.assertDoesNotThrow(
+ () -> metalake.revokeRolesFromGroup(Lists.newArrayList(roleName),
groupName));
+
+ // Clean up
+ metalake.removeGroup(groupName);
+ metalake.deleteRole(roleName);
+ }
+}
diff --git
a/integration-test/src/test/java/org/apache/gravitino/integration/test/authorization/OwnerPostHookIT.java
b/integration-test/src/test/java/org/apache/gravitino/integration/test/authorization/OwnerIT.java
similarity index 77%
rename from
integration-test/src/test/java/org/apache/gravitino/integration/test/authorization/OwnerPostHookIT.java
rename to
integration-test/src/test/java/org/apache/gravitino/integration/test/authorization/OwnerIT.java
index b7c6c1788..78411b6a3 100644
---
a/integration-test/src/test/java/org/apache/gravitino/integration/test/authorization/OwnerPostHookIT.java
+++
b/integration-test/src/test/java/org/apache/gravitino/integration/test/authorization/OwnerIT.java
@@ -33,6 +33,8 @@ import org.apache.gravitino.authorization.Privileges;
import org.apache.gravitino.authorization.SecurableObject;
import org.apache.gravitino.authorization.SecurableObjects;
import org.apache.gravitino.client.GravitinoMetalake;
+import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
+import org.apache.gravitino.exceptions.NotFoundException;
import org.apache.gravitino.file.Fileset;
import org.apache.gravitino.integration.test.container.ContainerSuite;
import org.apache.gravitino.integration.test.container.HiveContainer;
@@ -50,9 +52,9 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@Tag("gravitino-docker-test")
-public class OwnerPostHookIT extends AbstractIT {
+public class OwnerIT extends AbstractIT {
- private static final Logger LOG =
LoggerFactory.getLogger(OwnerPostHookIT.class);
+ private static final Logger LOG = LoggerFactory.getLogger(OwnerIT.class);
private static final ContainerSuite containerSuite =
ContainerSuite.getInstance();
private static String hmsUri;
@@ -116,12 +118,26 @@ public class OwnerPostHookIT extends AbstractIT {
Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, owner.name());
Assertions.assertEquals(Owner.Type.USER, owner.type());
+ // Set another owner
+ String anotherUser = "another";
+ metalake.addUser(anotherUser);
+ metalake.setOwner(metalakeObject, anotherUser, Owner.Type.USER);
+ owner = metalake.getOwner(metalakeObject).get();
+ Assertions.assertEquals(anotherUser, owner.name());
+ Assertions.assertEquals(Owner.Type.USER, owner.type());
+
MetadataObject catalogObject =
MetadataObjects.of(Lists.newArrayList(catalogNameA),
MetadataObject.Type.CATALOG);
owner = metalake.getOwner(catalogObject).get();
Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, owner.name());
Assertions.assertEquals(Owner.Type.USER, owner.type());
+ // Set another owner
+ metalake.setOwner(catalogObject, anotherUser, Owner.Type.USER);
+ owner = metalake.getOwner(catalogObject).get();
+ Assertions.assertEquals(anotherUser, owner.name());
+ Assertions.assertEquals(Owner.Type.USER, owner.type());
+
MetadataObject schemaObject =
MetadataObjects.of(
Lists.newArrayList(catalogNameA, "schema_owner"),
MetadataObject.Type.SCHEMA);
@@ -129,6 +145,12 @@ public class OwnerPostHookIT extends AbstractIT {
Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, owner.name());
Assertions.assertEquals(Owner.Type.USER, owner.type());
+ // Set another owner
+ metalake.setOwner(schemaObject, anotherUser, Owner.Type.USER);
+ owner = metalake.getOwner(schemaObject).get();
+ Assertions.assertEquals(anotherUser, owner.name());
+ Assertions.assertEquals(Owner.Type.USER, owner.type());
+
MetadataObject filesetObject =
MetadataObjects.of(
Lists.newArrayList(catalogNameA, "schema_owner", "fileset_owner"),
@@ -137,6 +159,12 @@ public class OwnerPostHookIT extends AbstractIT {
Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, owner.name());
Assertions.assertEquals(Owner.Type.USER, owner.type());
+ // Set another owner
+ metalake.setOwner(filesetObject, anotherUser, Owner.Type.USER);
+ owner = metalake.getOwner(catalogObject).get();
+ Assertions.assertEquals(anotherUser, owner.name());
+ Assertions.assertEquals(Owner.Type.USER, owner.type());
+
// Clean up
catalog.asFilesetCatalog().dropFileset(fileIdent);
catalog.asSchemas().dropSchema("schema_owner", true);
@@ -181,6 +209,14 @@ public class OwnerPostHookIT extends AbstractIT {
Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, owner.name());
Assertions.assertEquals(Owner.Type.USER, owner.type());
+ // Set another owner
+ String anotherUser = "another";
+ metalake.addUser(anotherUser);
+ metalake.setOwner(topicObject, anotherUser, Owner.Type.USER);
+ owner = metalake.getOwner(topicObject).get();
+ Assertions.assertEquals(anotherUser, owner.name());
+ Assertions.assertEquals(Owner.Type.USER, owner.type());
+
// Clean up
catalogB.asTopicCatalog().dropTopic(topicIdent);
metalake.dropCatalog(catalogNameB);
@@ -209,6 +245,14 @@ public class OwnerPostHookIT extends AbstractIT {
Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, owner.name());
Assertions.assertEquals(Owner.Type.USER, owner.type());
+ // Set another owner
+ String anotherUser = "another";
+ metalake.addUser(anotherUser);
+ metalake.setOwner(roleObject, anotherUser, Owner.Type.USER);
+ owner = metalake.getOwner(roleObject).get();
+ Assertions.assertEquals(anotherUser, owner.name());
+ Assertions.assertEquals(Owner.Type.USER, owner.type());
+
// Clean up
metalake.deleteRole("role_owner");
client.dropMetalake(metalakeNameC);
@@ -265,10 +309,49 @@ public class OwnerPostHookIT extends AbstractIT {
Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, owner.name());
Assertions.assertEquals(Owner.Type.USER, owner.type());
+ // Set another owner
+ String anotherUser = "another";
+ metalake.addUser(anotherUser);
+ metalake.setOwner(tableObject, anotherUser, Owner.Type.USER);
+ owner = metalake.getOwner(tableObject).get();
+ Assertions.assertEquals(anotherUser, owner.name());
+ Assertions.assertEquals(Owner.Type.USER, owner.type());
+
// Clean up
catalog.asTableCatalog().dropTable(tableIdent);
catalog.asSchemas().dropSchema("schema_owner", true);
metalake.dropCatalog(catalogNameD);
client.dropMetalake(metalakeNameD);
}
+
+ @Test
+ public void testOwnerWithException() {
+ String metalakeNameE = RandomNameUtils.genRandomName("metalakeE");
+ String catalogNameE = RandomNameUtils.genRandomName("catalogE");
+ GravitinoMetalake metalake =
+ client.createMetalake(metalakeNameE, "metalake E comment",
Collections.emptyMap());
+
+ MetadataObject metalakeObject =
+ MetadataObjects.of(Lists.newArrayList(metalakeNameE),
MetadataObject.Type.METALAKE);
+
+ MetadataObject catalogObject =
+ MetadataObjects.of(Lists.newArrayList(catalogNameE),
MetadataObject.Type.CATALOG);
+
+ // Get a not-existed catalog
+ Assertions.assertThrows(
+ NoSuchMetadataObjectException.class, () ->
metalake.getOwner(catalogObject));
+
+ // Set a not-existed catalog
+ Assertions.assertThrows(
+ NotFoundException.class,
+ () -> metalake.setOwner(catalogObject, AuthConstants.ANONYMOUS_USER,
Owner.Type.USER));
+
+ // Set a not-existed user
+ Assertions.assertThrows(
+ NotFoundException.class,
+ () -> metalake.setOwner(metalakeObject, "not-existed",
Owner.Type.USER));
+
+ // Cleanup
+ client.dropMetalake(metalakeNameE);
+ }
}
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/rest/RoleOperations.java
b/server/src/main/java/org/apache/gravitino/server/web/rest/RoleOperations.java
index f3e8e2f86..a3699c0f9 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/rest/RoleOperations.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/rest/RoleOperations.java
@@ -46,6 +46,7 @@ import org.apache.gravitino.dto.requests.RoleCreateRequest;
import org.apache.gravitino.dto.responses.DeleteResponse;
import org.apache.gravitino.dto.responses.RoleResponse;
import org.apache.gravitino.dto.util.DTOConverters;
+import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
import org.apache.gravitino.lock.LockType;
import org.apache.gravitino.lock.TreeLockUtils;
import org.apache.gravitino.metrics.MetricNames;
@@ -179,7 +180,7 @@ public class RoleOperations {
identifier = NameIdentifier.parse(String.format("%s.%s", metalake,
object.fullName()));
}
- String existErrMsg = "Securable object % doesn't exist";
+ String existErrMsg = "Securable object %s doesn't exist";
TreeLockUtils.doWithTreeLock(
identifier,
@@ -188,41 +189,41 @@ public class RoleOperations {
switch (object.type()) {
case METALAKE:
if
(!GravitinoEnv.getInstance().metalakeDispatcher().metalakeExists(identifier)) {
- throw new IllegalArgumentException(String.format(existErrMsg,
object.fullName()));
+ throw new NoSuchMetadataObjectException(existErrMsg,
object.fullName());
}
break;
case CATALOG:
if
(!GravitinoEnv.getInstance().catalogDispatcher().catalogExists(identifier)) {
- throw new IllegalArgumentException(String.format(existErrMsg,
object.fullName()));
+ throw new NoSuchMetadataObjectException(existErrMsg,
object.fullName());
}
break;
case SCHEMA:
if
(!GravitinoEnv.getInstance().schemaDispatcher().schemaExists(identifier)) {
- throw new IllegalArgumentException(String.format(existErrMsg,
object.fullName()));
+ throw new NoSuchMetadataObjectException(existErrMsg,
object.fullName());
}
break;
case FILESET:
if
(!GravitinoEnv.getInstance().filesetDispatcher().filesetExists(identifier)) {
- throw new IllegalArgumentException(String.format(existErrMsg,
object.fullName()));
+ throw new NoSuchMetadataObjectException(existErrMsg,
object.fullName());
}
break;
case TABLE:
if
(!GravitinoEnv.getInstance().tableDispatcher().tableExists(identifier)) {
- throw new IllegalArgumentException(String.format(existErrMsg,
object.fullName()));
+ throw new NoSuchMetadataObjectException(existErrMsg,
object.fullName());
}
break;
case TOPIC:
if
(!GravitinoEnv.getInstance().topicDispatcher().topicExists(identifier)) {
- throw new IllegalArgumentException(String.format(existErrMsg,
object.fullName()));
+ throw new NoSuchMetadataObjectException(existErrMsg,
object.fullName());
}
break;
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestRoleOperations.java
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestRoleOperations.java
index 6c34547d0..a00f91b4d 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestRoleOperations.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestRoleOperations.java
@@ -56,6 +56,7 @@ import org.apache.gravitino.dto.responses.ErrorConstants;
import org.apache.gravitino.dto.responses.ErrorResponse;
import org.apache.gravitino.dto.responses.RoleResponse;
import org.apache.gravitino.dto.util.DTOConverters;
+import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
import org.apache.gravitino.exceptions.NoSuchMetalakeException;
import org.apache.gravitino.exceptions.NoSuchRoleException;
import org.apache.gravitino.exceptions.RoleAlreadyExistsException;
@@ -199,10 +200,10 @@ public class TestRoleOperations extends JerseyTest {
.request(MediaType.APPLICATION_JSON_TYPE)
.accept("application/vnd.gravitino.v1+json")
.post(Entity.entity(req, MediaType.APPLICATION_JSON_TYPE));
- Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
respNotExist.getStatus());
+ Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(),
respNotExist.getStatus());
Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
respNotExist.getMediaType());
ErrorResponse notExistResponse =
respNotExist.readEntity(ErrorResponse.class);
- Assertions.assertEquals(ErrorConstants.ILLEGAL_ARGUMENTS_CODE,
notExistResponse.getCode());
+ Assertions.assertEquals(ErrorConstants.NOT_FOUND_CODE,
notExistResponse.getCode());
// Test to throw NoSuchMetalakeException
when(catalogDispatcher.catalogExists(any())).thenReturn(true);
@@ -402,7 +403,7 @@ public class TestRoleOperations extends JerseyTest {
() -> RoleOperations.checkSecurableObject("metalake",
DTOConverters.toDTO(catalog)));
when(catalogDispatcher.catalogExists(any())).thenReturn(false);
Assertions.assertThrows(
- IllegalArgumentException.class,
+ NoSuchMetadataObjectException.class,
() -> RoleOperations.checkSecurableObject("metalake",
DTOConverters.toDTO(catalog)));
// check the schema
@@ -414,7 +415,7 @@ public class TestRoleOperations extends JerseyTest {
() -> RoleOperations.checkSecurableObject("metalake",
DTOConverters.toDTO(schema)));
when(schemaDispatcher.schemaExists(any())).thenReturn(false);
Assertions.assertThrows(
- IllegalArgumentException.class,
+ NoSuchMetadataObjectException.class,
() -> RoleOperations.checkSecurableObject("metalake",
DTOConverters.toDTO(schema)));
// check the table
@@ -426,7 +427,7 @@ public class TestRoleOperations extends JerseyTest {
() -> RoleOperations.checkSecurableObject("metalake",
DTOConverters.toDTO(table)));
when(tableDispatcher.tableExists(any())).thenReturn(false);
Assertions.assertThrows(
- IllegalArgumentException.class,
+ NoSuchMetadataObjectException.class,
() -> RoleOperations.checkSecurableObject("metalake",
DTOConverters.toDTO(table)));
// check the topic
@@ -438,7 +439,7 @@ public class TestRoleOperations extends JerseyTest {
() -> RoleOperations.checkSecurableObject("metalake",
DTOConverters.toDTO(topic)));
when(topicDispatcher.topicExists(any())).thenReturn(false);
Assertions.assertThrows(
- IllegalArgumentException.class,
+ NoSuchMetadataObjectException.class,
() -> RoleOperations.checkSecurableObject("metalake",
DTOConverters.toDTO(topic)));
// check the fileset
@@ -450,7 +451,7 @@ public class TestRoleOperations extends JerseyTest {
() -> RoleOperations.checkSecurableObject("metalake",
DTOConverters.toDTO(fileset)));
when(filesetDispatcher.filesetExists(any())).thenReturn(false);
Assertions.assertThrows(
- IllegalArgumentException.class,
+ NoSuchMetadataObjectException.class,
() -> RoleOperations.checkSecurableObject("metalake",
DTOConverters.toDTO(fileset)));
}
}
