[#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
apache · Aug 14, 2024 · d219959 · d219959
1 parent 33f8919
commit d219959
Show file tree

Hide file tree

Showing 8 changed files with 427 additions and 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
@@ -627,6 +627,10 @@ public void accept(ErrorResponse errorResponse) {
             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
@@ -242,11 +242,12 @@ public boolean deleteRole(String role) throws NoSuchMetalakeException {
    * @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
@@ -82,6 +82,7 @@
 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 User addUser(String user) throws UserAlreadyExistsException, NoSuchMetala
   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 boolean removeUser(String user) throws NoSuchMetalakeException {
   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 Group addGroup(String group) throws GroupAlreadyExistsException, NoSuchMe
   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 boolean removeGroup(String group) throws NoSuchMetalakeException {
   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 Group getGroup(String group) throws NoSuchGroupException, NoSuchMetalakeE
   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 Role getRole(String role) throws NoSuchRoleException, NoSuchMetalakeExcep
   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 boolean deleteRole(String role) throws NoSuchMetalakeException {
    * @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 User grantRolesToUser(List<String> roles, String user)
 
     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 Group grantRolesToGroup(List<String> roles, String group)
     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 User revokeRolesFromUser(List<String> roles, String user)
 
     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 Group revokeRolesFromGroup(List<String> roles, String group)
     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 Optional<Owner> getOwner(MetadataObject object) throws NoSuchMetadataObje
                 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 void setOwner(MetadataObject object, String ownerName, Owner.Type ownerTy
                 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
@@ -31,9 +31,6 @@
 /** 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 String name() {
     return name;
   }
 
-  @Override
-  public String fullName() {
-    return fullName;
-  }
-
   @Override
   public Type type() {
     return type;
@@ -106,10 +98,13 @@ public static Builder builder() {
 
   /** 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 SecurableObjectDTO build() {
       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;
     }
   }
 }