[#6042] refactor: Delete the privilege of catalog after dropping the …

…catalogs (#6045) ### What changes were proposed in this pull request? Delete the privilege of catalog after dropping the catalogs ### Why are the changes needed? Fix: #6042 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? CI passed. --------- Co-authored-by: Qiming Teng <[email protected]>
apache · Jan 9, 2025 · 31a60e5 · 31a60e5
1 parent 598bc05
commit 31a60e5
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 19 deletions.
diff --git a/...test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerBaseE2EIT.java b/...test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerBaseE2EIT.java
@@ -143,6 +143,8 @@ protected void cleanIT() {
               (schema -> {
                 catalog.asSchemas().dropSchema(schema, false);
               }));
+
+      // The `dropCatalog` call will invoke the catalog metadata object to remove privileges
       Arrays.stream(metalake.listCatalogs())
           .forEach((catalogName -> metalake.dropCatalog(catalogName, true)));
       client.disableMetalake(metalakeName);

diff --git a/core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java b/core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java
@@ -33,6 +33,7 @@
 import org.apache.gravitino.Entity;
 import org.apache.gravitino.GravitinoEnv;
 import org.apache.gravitino.MetadataObject;
+import org.apache.gravitino.MetadataObjects;
 import org.apache.gravitino.NameIdentifier;
 import org.apache.gravitino.Namespace;
 import org.apache.gravitino.Schema;
@@ -186,19 +187,8 @@ public static void callAuthorizationPluginForSecurableObjects(
 
   public static void callAuthorizationPluginForMetadataObject(
       String metalake, MetadataObject metadataObject, Consumer<AuthorizationPlugin> consumer) {
-    CatalogManager catalogManager = GravitinoEnv.getInstance().catalogManager();
-    if (needApplyAuthorizationPluginAllCatalogs(metadataObject.type())) {
-      NameIdentifier[] catalogs = catalogManager.listCatalogs(Namespace.of(metalake));
-      // ListCatalogsInfo return `CatalogInfo` instead of `BaseCatalog`, we need `BaseCatalog` to
-      // call authorization plugin method.
-      for (NameIdentifier catalog : catalogs) {
-        callAuthorizationPluginImpl(consumer, catalogManager.loadCatalog(catalog));
-      }
-    } else if (needApplyAuthorization(metadataObject.type())) {
-      NameIdentifier catalogIdent =
-          NameIdentifierUtil.getCatalogIdentifier(
-              MetadataObjectUtil.toEntityIdent(metalake, metadataObject));
-      Catalog catalog = catalogManager.loadCatalog(catalogIdent);
+    List<Catalog> loadedCatalogs = loadMetadataObjectCatalog(metalake, metadataObject);
+    for (Catalog catalog : loadedCatalogs) {
       callAuthorizationPluginImpl(consumer, catalog);
     }
   }
@@ -266,16 +256,33 @@ public static void authorizationPluginRemovePrivileges(
     // authorization plugin.
     if (GravitinoEnv.getInstance().accessControlDispatcher() != null) {
       MetadataObject metadataObject = NameIdentifierUtil.toMetadataObject(ident, type);
+      String metalake =
+          type == Entity.EntityType.METALAKE ? ident.name() : ident.namespace().level(0);
+
       MetadataObjectChange removeObject = MetadataObjectChange.remove(metadataObject, locations);
       callAuthorizationPluginForMetadataObject(
-          ident.namespace().level(0),
+          metalake,
           metadataObject,
           authorizationPlugin -> {
             authorizationPlugin.onMetadataUpdated(removeObject);
           });
     }
   }
 
+  public static void removeCatalogPrivileges(Catalog catalog, List<String> locations) {
+    // If we enable authorization, we should remove the privileges about the entity in the
+    // authorization plugin.
+    MetadataObject metadataObject =
+        MetadataObjects.of(null, catalog.name(), MetadataObject.Type.CATALOG);
+    MetadataObjectChange removeObject = MetadataObjectChange.remove(metadataObject, locations);
+
+    callAuthorizationPluginImpl(
+        authorizationPlugin -> {
+          authorizationPlugin.onMetadataUpdated(removeObject);
+        },
+        catalog);
+  }
+
   public static void authorizationPluginRenamePrivileges(
       NameIdentifier ident, Entity.EntityType type, String newName) {
     // If we enable authorization, we should rename the privileges about the entity in the
@@ -377,6 +384,28 @@ private static void checkCatalogType(
     }
   }
 
+  private static List<Catalog> loadMetadataObjectCatalog(
+      String metalake, MetadataObject metadataObject) {
+    CatalogManager catalogManager = GravitinoEnv.getInstance().catalogManager();
+    List<Catalog> loadedCatalogs = Lists.newArrayList();
+    if (needApplyAuthorizationPluginAllCatalogs(metadataObject.type())) {
+      NameIdentifier[] catalogs = catalogManager.listCatalogs(Namespace.of(metalake));
+      // ListCatalogsInfo return `CatalogInfo` instead of `BaseCatalog`, we need `BaseCatalog` to
+      // call authorization plugin method.
+      for (NameIdentifier catalog : catalogs) {
+        loadedCatalogs.add(catalogManager.loadCatalog(catalog));
+      }
+    } else if (needApplyAuthorization(metadataObject.type())) {
+      NameIdentifier catalogIdent =
+          NameIdentifierUtil.getCatalogIdentifier(
+              MetadataObjectUtil.toEntityIdent(metalake, metadataObject));
+      Catalog catalog = catalogManager.loadCatalog(catalogIdent);
+      loadedCatalogs.add(catalog);
+    }
+
+    return loadedCatalogs;
+  }
+
   // The Hive default schema location is Hive warehouse directory
   private static String getHiveDefaultLocation(String metalakeName, String catalogName) {
     NameIdentifier defaultSchemaIdent =

diff --git a/core/src/main/java/org/apache/gravitino/hook/CatalogHookDispatcher.java b/core/src/main/java/org/apache/gravitino/hook/CatalogHookDispatcher.java
@@ -127,11 +127,22 @@ public boolean dropCatalog(NameIdentifier ident) {
   @Override
   public boolean dropCatalog(NameIdentifier ident, boolean force)
       throws NonEmptyEntityException, CatalogInUseException {
-    List<String> locations =
-        AuthorizationUtils.getMetadataObjectLocation(ident, Entity.EntityType.CATALOG);
-    AuthorizationUtils.authorizationPluginRemovePrivileges(
-        ident, Entity.EntityType.CATALOG, locations);
-    return dispatcher.dropCatalog(ident, force);
+    if (!dispatcher.catalogExists(ident)) {
+      return false;
+    }
+
+    // If we call the authorization plugin after dropping catalog, we can't load the plugin of the
+    // catalog
+    Catalog catalog = dispatcher.loadCatalog(ident);
+    boolean dropped = dispatcher.dropCatalog(ident, force);
+
+    if (dropped && catalog != null) {
+      List<String> locations =
+          AuthorizationUtils.getMetadataObjectLocation(ident, Entity.EntityType.CATALOG);
+      AuthorizationUtils.removeCatalogPrivileges(catalog, locations);
+    }
+
+    return dropped;
   }
 
   @Override