address comment

apache · Oct 8, 2024 · b6802de · b6802de
1 parent f7c681a
commit b6802de
Show file tree

Hide file tree

Showing 14 changed files with 280 additions and 127 deletions.
diff --git a/api/src/main/java/org/apache/gravitino/exceptions/IllegalPrivilegeException.java b/api/src/main/java/org/apache/gravitino/exceptions/IllegalPrivilegeException.java
@@ -0,0 +1,62 @@
+/*
+ * 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.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+import com.google.errorprone.annotations.FormatString;
+
+/** An exception thrown when a privilege is invalid. */
+public class IllegalPrivilegeException extends IllegalArgumentException {
+  /**
+   * Constructs a new exception with the specified detail message.
+   *
+   * @param message the detail message.
+   * @param args the arguments to the message.
+   */
+  @FormatMethod
+  public IllegalPrivilegeException(@FormatString String message, Object... args) {
+    super(String.format(message, args));
+  }
+
+  /**
+   * Constructs a new exception with the specified detail message and cause.
+   *
+   * @param cause the cause.
+   * @param message the detail message.
+   * @param args the arguments to the message.
+   */
+  @FormatMethod
+  public IllegalPrivilegeException(Throwable cause, @FormatString String message, Object... args) {
+    super(String.format(message, args), cause);
+  }
+
+  /**
+   * Constructs a new exception with the specified cause.
+   *
+   * @param cause the cause.
+   */
+  public IllegalPrivilegeException(Throwable cause) {
+    super(cause);
+  }
+
+  /** Constructs a new exception with the specified detail message and cause. */
+  public IllegalPrivilegeException() {
+    super();
+  }
+}
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
@@ -31,6 +31,7 @@
 import org.apache.gravitino.exceptions.ConnectionFailedException;
 import org.apache.gravitino.exceptions.FilesetAlreadyExistsException;
 import org.apache.gravitino.exceptions.GroupAlreadyExistsException;
+import org.apache.gravitino.exceptions.IllegalPrivilegeException;
 import org.apache.gravitino.exceptions.MetalakeAlreadyExistsException;
 import org.apache.gravitino.exceptions.NoSuchCatalogException;
 import org.apache.gravitino.exceptions.NoSuchFilesetException;
@@ -626,7 +627,11 @@ public void accept(ErrorResponse errorResponse) {
 
       switch (errorResponse.getCode()) {
         case ErrorConstants.ILLEGAL_ARGUMENTS_CODE:
-          throw new IllegalArgumentException(errorMessage);
+          if (errorResponse.getType().equals(IllegalPrivilegeException.class.getSimpleName())) {
+            throw new IllegalPrivilegeException(errorMessage);
+          } else {
+            throw new IllegalArgumentException(errorMessage);
+          }
 
         case ErrorConstants.NOT_FOUND_CODE:
           if (errorResponse.getType().equals(NoSuchMetalakeException.class.getSimpleName())) {
@@ -669,7 +674,11 @@ public void accept(ErrorResponse errorResponse) {
 
       switch (errorResponse.getCode()) {
         case ErrorConstants.ILLEGAL_ARGUMENTS_CODE:
-          throw new IllegalArgumentException(errorMessage);
+          if (errorResponse.getType().equals(IllegalPrivilegeException.class.getSimpleName())) {
+            throw new IllegalPrivilegeException(errorMessage);
+          } else {
+            throw new IllegalArgumentException(errorMessage);
+          }
 
         case ErrorConstants.NOT_FOUND_CODE:
           if (errorResponse.getType().equals(NoSuchMetalakeException.class.getSimpleName())) {

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
@@ -35,6 +35,7 @@
 import org.apache.gravitino.authorization.User;
 import org.apache.gravitino.exceptions.CatalogAlreadyExistsException;
 import org.apache.gravitino.exceptions.GroupAlreadyExistsException;
+import org.apache.gravitino.exceptions.IllegalPrivilegeException;
 import org.apache.gravitino.exceptions.NoSuchCatalogException;
 import org.apache.gravitino.exceptions.NoSuchGroupException;
 import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
@@ -401,11 +402,12 @@ public String[] listRoleNames() throws NoSuchMetalakeException {
    * @throws NoSuchMetadataObjectException If the metadata object with the given name does not
    *     exist.
    * @throws NoSuchMetalakeException If the Metalake with the given name does not exist.
-   * @throws IllegalArgumentException If any privilege can't be bind to the metadata object.
+   * @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object.
    * @throws RuntimeException If granting roles to a role encounters storage issues.
    */
   public Role grantPrivilegesToRole(String role, MetadataObject object, List<Privilege> privileges)
-      throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException {
+      throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException,
+          IllegalPrivilegeException {
     return getMetalake().grantPrivilegesToRole(role, object, privileges);
   }
 
@@ -420,12 +422,13 @@ public Role grantPrivilegesToRole(String role, MetadataObject object, List<Privi
    * @throws NoSuchMetadataObjectException If the metadata object with the given name does not
    *     exist.
    * @throws NoSuchMetalakeException If the Metalake with the given name does not exist.
-   * @throws IllegalArgumentException If any privilege can't be bind to the metadata object.
+   * @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object.
    * @throws RuntimeException If revoking privileges from a role encounters storage issues.
    */
   public Role revokePrivilegesFromRole(
       String role, MetadataObject object, List<Privilege> privileges)
-      throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException {
+      throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException,
+          IllegalPrivilegeException {
     return getMetalake().revokePrivilegesFromRole(role, object, privileges);
   }
 

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
@@ -78,6 +78,7 @@
 import org.apache.gravitino.dto.responses.UserResponse;
 import org.apache.gravitino.exceptions.CatalogAlreadyExistsException;
 import org.apache.gravitino.exceptions.GroupAlreadyExistsException;
+import org.apache.gravitino.exceptions.IllegalPrivilegeException;
 import org.apache.gravitino.exceptions.NoSuchCatalogException;
 import org.apache.gravitino.exceptions.NoSuchGroupException;
 import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
@@ -908,11 +909,12 @@ public Group revokeRolesFromGroup(List<String> roles, String group)
    * @throws NoSuchMetadataObjectException If the metadata object with the given name does not
    *     exist.
    * @throws NoSuchMetalakeException If the Metalake with the given name does not exist.
-   * @throws IllegalArgumentException If any privilege can't be bind to the metadata object.
+   * @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object.
    * @throws RuntimeException If granting privileges to a role encounters storage issues.
    */
   public Role grantPrivilegesToRole(String role, MetadataObject object, List<Privilege> privileges)
-      throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException {
+      throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException,
+          IllegalPrivilegeException {
     PrivilegeGrantRequest request =
         new PrivilegeGrantRequest(DTOConverters.toPrivileges(privileges));
     request.validate();
@@ -948,12 +950,13 @@ public Role grantPrivilegesToRole(String role, MetadataObject object, List<Privi
    * @throws NoSuchMetadataObjectException If the metadata object with the given name does not
    *     exist.
    * @throws NoSuchMetalakeException If the Metalake with the given name does not exist.
-   * @throws IllegalArgumentException If any privilege can't be bind to the metadata object.
+   * @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object.
    * @throws RuntimeException If revoking privileges from a role encounters storage issues.
    */
   public Role revokePrivilegesFromRole(
       String role, MetadataObject object, List<Privilege> privileges)
-      throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException {
+      throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException,
+          IllegalPrivilegeException {
     PrivilegeRevokeRequest request =
         new PrivilegeRevokeRequest(DTOConverters.toPrivileges(privileges));
     request.validate();

diff --git a/...test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java b/...test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java
@@ -42,6 +42,7 @@
 import org.apache.gravitino.authorization.User;
 import org.apache.gravitino.client.GravitinoMetalake;
 import org.apache.gravitino.exceptions.GroupAlreadyExistsException;
+import org.apache.gravitino.exceptions.IllegalPrivilegeException;
 import org.apache.gravitino.exceptions.NoSuchGroupException;
 import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
 import org.apache.gravitino.exceptions.NoSuchRoleException;
@@ -453,7 +454,7 @@ void testManageRolePermissions() {
     // grant a wrong privilege
     MetadataObject catalog = MetadataObjects.of(null, "catalog", MetadataObject.Type.CATALOG);
     Assertions.assertThrows(
-        IllegalArgumentException.class,
+        IllegalPrivilegeException.class,
         () ->
             metalake.grantPrivilegesToRole(
                 roleName, catalog, Lists.newArrayList(Privileges.CreateCatalog.allow())));
@@ -462,7 +463,7 @@ void testManageRolePermissions() {
     MetadataObject wrongCatalog =
         MetadataObjects.of(null, "fileset_catalog", MetadataObject.Type.CATALOG);
     Assertions.assertThrows(
-        IllegalArgumentException.class,
+        IllegalPrivilegeException.class,
         () ->
             metalake.grantPrivilegesToRole(
                 roleName, wrongCatalog, Lists.newArrayList(Privileges.SelectTable.allow())));
@@ -488,14 +489,14 @@ void testManageRolePermissions() {
 
     // revoke a wrong privilege
     Assertions.assertThrows(
-        IllegalArgumentException.class,
+        IllegalPrivilegeException.class,
         () ->
             metalake.revokePrivilegesFromRole(
                 roleName, catalog, Lists.newArrayList(Privileges.CreateCatalog.allow())));
 
     // revoke a wrong catalog type privilege
     Assertions.assertThrows(
-        IllegalArgumentException.class,
+        IllegalPrivilegeException.class,
         () ->
             metalake.revokePrivilegesFromRole(
                 roleName, wrongCatalog, Lists.newArrayList(Privileges.SelectTable.allow())));

diff --git a/common/src/main/java/org/apache/gravitino/dto/responses/ErrorResponse.java b/common/src/main/java/org/apache/gravitino/dto/responses/ErrorResponse.java
@@ -119,11 +119,20 @@ public static ErrorResponse illegalArguments(String message) {
    * @return The new instance.
    */
   public static ErrorResponse illegalArguments(String message, Throwable throwable) {
+    return illegalArguments(IllegalArgumentException.class.getSimpleName(), message, throwable);
+  }
+
+  /**
+   * Create a new illegal arguments error instance of {@link ErrorResponse}.
+   *
+   * @param type The type of the error.
+   * @param message The message of the error.
+   * @param throwable The throwable that caused the error.
+   * @return The new instance.
+   */
+  public static ErrorResponse illegalArguments(String type, String message, Throwable throwable) {
     return new ErrorResponse(
-        ErrorConstants.ILLEGAL_ARGUMENTS_CODE,
-        IllegalArgumentException.class.getSimpleName(),
-        message,
-        getStackTrace(throwable));
+        ErrorConstants.ILLEGAL_ARGUMENTS_CODE, type, message, getStackTrace(throwable));
   }
 
   /**

diff --git a/core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java b/core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java
@@ -22,6 +22,7 @@
 
 import com.google.common.collect.Sets;
 import java.io.IOException;
+import java.util.Collection;
 import java.util.List;
 import java.util.Set;
 import java.util.function.Consumer;
@@ -38,6 +39,7 @@
 import org.apache.gravitino.connector.authorization.AuthorizationPlugin;
 import org.apache.gravitino.dto.authorization.PrivilegeDTO;
 import org.apache.gravitino.dto.util.DTOConverters;
+import org.apache.gravitino.exceptions.IllegalPrivilegeException;
 import org.apache.gravitino.exceptions.NoSuchCatalogException;
 import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
 import org.apache.gravitino.exceptions.NoSuchMetalakeException;
@@ -266,15 +268,25 @@ public static void checkSecurableObject(String metalake, MetadataObject object)
     }
   }
 
+  public static void checkDuplicatedNamePrivilege(Collection<Privilege> privileges) {
+    Set<Privilege.Name> privilegeNameSet = Sets.newHashSet();
+    for (Privilege privilege : privileges) {
+      if (privilegeNameSet.contains(privilege.name())) {
+        throw new IllegalPrivilegeException(
+            "Doesn't support duplicated privilege name %s", privilege.name());
+      }
+      privilegeNameSet.add(privilege.name());
+    }
+  }
+
   public static void checkPrivilege(
       PrivilegeDTO privilegeDTO, MetadataObject object, String metalake) {
     Privilege privilege = DTOConverters.fromPrivilegeDTO(privilegeDTO);
 
     if (!privilege.canBindTo(object.type())) {
-      throw new IllegalArgumentException(
-          String.format(
-              "Securable object %s type %s don't support binding privilege %s",
-              object.fullName(), object.type(), privilege));
+      throw new IllegalPrivilegeException(
+          "Securable object %s type %s don't support binding privilege %s",
+          object.fullName(), object.type(), privilege);
     }
 
     if (object.type() == MetadataObject.Type.CATALOG
@@ -311,10 +323,8 @@ private static void checkCatalogType(
       NameIdentifier catalogIdent, Catalog.Type type, Privilege privilege) {
     Catalog catalog = GravitinoEnv.getInstance().catalogDispatcher().loadCatalog(catalogIdent);
     if (catalog.type() != type) {
-      throw new IllegalArgumentException(
-          String.format(
-              "Catalog %s type %s don't support privilege %s",
-              catalogIdent, catalog.type(), privilege));
+      throw new IllegalPrivilegeException(
+          "Catalog %s type %s don't support privilege %s", catalogIdent, catalog.type(), privilege);
     }
   }