This is an automated email from the ASF dual-hosted git repository. github-actions[bot] pushed a commit to branch cherry-pick-06a5a4f3-to-branch-1.3 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 6847026809591f7a959e9e0b5010c2caca082eef Author: Qi Yu <[email protected]> AuthorDate: Wed Sep 16 19:32:47 2026 +0800 [#13174] fix(core): Keep plugin privileges when the external drop returns false (#13196) ### What changes were proposed in this pull request? `TableHookDispatcher.dropTable`/`purgeTable`, `SchemaHookDispatcher.dropSchema`, `FilesetHookDispatcher.dropFileset` and `TopicHookDispatcher.dropTopic` now call `AuthorizationUtils.authorizationPluginRemovePrivileges` only when the inner dispatcher returned `true`. Adds one test per hook that verifies a `false` result leaves the plugin untouched. ### Why are the changes needed? #12232 made the dispatchers keep the store registration when the external drop returns `false` (the object was renamed or dropped out of band), but the hook layer still removed the authorization-plugin privileges unconditionally. Under a concurrent rename × drop the entity alive under its new name lost its Ranger privileges while keeping its Gravitino registration. `FunctionHookDispatcher` already had the guard; this aligns the other four. Fix: #13174 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? New unit tests `testDrop*KeepsPrivilegesWhen*ReturnsFalse` in the four `Test*HookDispatcher` classes; all `*HookDispatcher` tests pass (`./gradlew :core:test --tests '*HookDispatcher' -PskipITs`). --- .../ranger/integration/test/RangerBaseE2EIT.java | 20 +++++++++++++--- .../gravitino/hook/FilesetHookDispatcher.java | 7 ++++-- .../gravitino/hook/SchemaHookDispatcher.java | 8 +++++-- .../apache/gravitino/hook/TableHookDispatcher.java | 14 +++++++---- .../apache/gravitino/hook/TopicHookDispatcher.java | 7 ++++-- .../gravitino/hook/TestFilesetHookDispatcher.java | 23 ++++++++++++++++++ .../gravitino/hook/TestSchemaHookDispatcher.java | 22 +++++++++++++++++ .../gravitino/hook/TestTableHookDispatcher.java | 28 ++++++++++++++++++++++ .../gravitino/hook/TestTopicHookDispatcher.java | 23 ++++++++++++++++++ 9 files changed, 139 insertions(+), 13 deletions(-) diff --git a/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerBaseE2EIT.java b/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerBaseE2EIT.java index 3c38ba0908..e404e77a36 100644 --- a/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerBaseE2EIT.java +++ b/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerBaseE2EIT.java @@ -1002,8 +1002,16 @@ public abstract class RangerBaseE2EIT extends BaseIT { // Owner has all the privileges except for creating table checkTableAllPrivilegesExceptForCreating(); - // Delete Gravitino's meta data - catalog.asTableCatalog().purgeTable(NameIdentifier.of(schemaName, tableName)); + // The table was dropped externally, so Gravitino keeps its registration and privileges. + Assertions.assertFalse( + catalog.asTableCatalog().purgeTable(NameIdentifier.of(schemaName, tableName))); + waitForUpdatingPolicies(); + + // The retained owner privileges allow recreating the same table. A successful Gravitino purge + // then removes both its registration and privileges. + sparkSession.sql(SQL_CREATE_TABLE); + Assertions.assertTrue( + catalog.asTableCatalog().purgeTable(NameIdentifier.of(schemaName, tableName))); waitForUpdatingPolicies(); checker.checkCreateTable(); @@ -1022,7 +1030,13 @@ public abstract class RangerBaseE2EIT extends BaseIT { // Succeed to drop schema sparkSession.sql(SQL_DROP_SCHEMA); - catalog.asSchemas().dropSchema(schemaName, false); + Assertions.assertFalse(catalog.asSchemas().dropSchema(schemaName, false)); + waitForUpdatingPolicies(); + + // The retained owner privileges allow recreating the same schema. A successful Gravitino drop + // then removes both its registration and privileges. + sparkSession.sql(SQL_CREATE_SCHEMA); + Assertions.assertTrue(catalog.asSchemas().dropSchema(schemaName, false)); waitForUpdatingPolicies(); checker.checkCreateSchema(); diff --git a/core/src/main/java/org/apache/gravitino/hook/FilesetHookDispatcher.java b/core/src/main/java/org/apache/gravitino/hook/FilesetHookDispatcher.java index d4794808ca..8db133b7b7 100644 --- a/core/src/main/java/org/apache/gravitino/hook/FilesetHookDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/hook/FilesetHookDispatcher.java @@ -114,8 +114,11 @@ public class FilesetHookDispatcher implements FilesetDispatcher { List<String> locations = AuthorizationUtils.getMetadataObjectLocation(ident, Entity.EntityType.FILESET); boolean dropped = dispatcher.dropFileset(ident); - AuthorizationUtils.authorizationPluginRemovePrivileges( - ident, Entity.EntityType.FILESET, locations); + // A false result means the fileset was already gone and the registration was kept. + if (dropped) { + AuthorizationUtils.authorizationPluginRemovePrivileges( + ident, Entity.EntityType.FILESET, locations); + } return dropped; } diff --git a/core/src/main/java/org/apache/gravitino/hook/SchemaHookDispatcher.java b/core/src/main/java/org/apache/gravitino/hook/SchemaHookDispatcher.java index 665787d251..d4735ffe26 100644 --- a/core/src/main/java/org/apache/gravitino/hook/SchemaHookDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/hook/SchemaHookDispatcher.java @@ -162,8 +162,12 @@ public class SchemaHookDispatcher implements SchemaDispatcher { List<String> locations = AuthorizationUtils.getMetadataObjectLocation(ident, Entity.EntityType.SCHEMA); boolean dropped = dispatcher.dropSchema(ident, cascade); - AuthorizationUtils.authorizationPluginRemovePrivileges( - ident, Entity.EntityType.SCHEMA, locations); + // Only a schema that was really dropped loses its privileges; a false result means the + // registration was kept. + if (dropped) { + AuthorizationUtils.authorizationPluginRemovePrivileges( + ident, Entity.EntityType.SCHEMA, locations); + } return dropped; } diff --git a/core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java b/core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java index 9a1037e1ca..768c1e5410 100644 --- a/core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java @@ -129,8 +129,12 @@ public class TableHookDispatcher implements TableDispatcher { List<String> locations = AuthorizationUtils.getMetadataObjectLocation(ident, Entity.EntityType.TABLE); boolean dropped = dispatcher.dropTable(ident); - AuthorizationUtils.authorizationPluginRemovePrivileges( - ident, Entity.EntityType.TABLE, locations); + // A false result means the external table was renamed or dropped out of band and the + // registration was kept; the entity alive under another name must keep its privileges too. + if (dropped) { + AuthorizationUtils.authorizationPluginRemovePrivileges( + ident, Entity.EntityType.TABLE, locations); + } return dropped; } @@ -139,8 +143,10 @@ public class TableHookDispatcher implements TableDispatcher { List<String> locations = AuthorizationUtils.getMetadataObjectLocation(ident, Entity.EntityType.TABLE); boolean purged = dispatcher.purgeTable(ident); - AuthorizationUtils.authorizationPluginRemovePrivileges( - ident, Entity.EntityType.TABLE, locations); + if (purged) { + AuthorizationUtils.authorizationPluginRemovePrivileges( + ident, Entity.EntityType.TABLE, locations); + } return purged; } diff --git a/core/src/main/java/org/apache/gravitino/hook/TopicHookDispatcher.java b/core/src/main/java/org/apache/gravitino/hook/TopicHookDispatcher.java index ad8440942b..4840cd87ae 100644 --- a/core/src/main/java/org/apache/gravitino/hook/TopicHookDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/hook/TopicHookDispatcher.java @@ -88,8 +88,11 @@ public class TopicHookDispatcher implements TopicDispatcher { List<String> locations = AuthorizationUtils.getMetadataObjectLocation(ident, Entity.EntityType.TOPIC); boolean dropped = dispatcher.dropTopic(ident); - AuthorizationUtils.authorizationPluginRemovePrivileges( - ident, Entity.EntityType.TOPIC, locations); + // A false result means the topic was dropped out of band and the registration was kept. + if (dropped) { + AuthorizationUtils.authorizationPluginRemovePrivileges( + ident, Entity.EntityType.TOPIC, locations); + } return dropped; } diff --git a/core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java b/core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java index 2710ff8748..0852b046bb 100644 --- a/core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java +++ b/core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java @@ -40,6 +40,7 @@ import static org.apache.gravitino.Configs.VERSION_RETENTION_COUNT; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import java.util.Map; @@ -50,6 +51,7 @@ import org.apache.gravitino.MetadataObject; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.Namespace; import org.apache.gravitino.authorization.AccessControlManager; +import org.apache.gravitino.authorization.AuthorizationUtils; import org.apache.gravitino.authorization.Owner; import org.apache.gravitino.authorization.OwnerDispatcher; import org.apache.gravitino.catalog.CatalogManager; @@ -69,6 +71,7 @@ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; +import org.mockito.MockedStatic; import org.mockito.Mockito; public class TestFilesetHookDispatcher extends TestOperationDispatcher { @@ -189,6 +192,26 @@ public class TestFilesetHookDispatcher extends TestOperationDispatcher { } } + @Test + public void testDropKeepsPrivilegesWhenDropReturnsFalse() { + FilesetDispatcher dispatcher = Mockito.mock(FilesetDispatcher.class); + FilesetHookDispatcher hook = new FilesetHookDispatcher(dispatcher); + NameIdentifier ident = NameIdentifier.of(metalake, catalog, "schema", "fileset"); + Mockito.when(dispatcher.dropFileset(ident)).thenReturn(false); + + try (MockedStatic<AuthorizationUtils> authz = Mockito.mockStatic(AuthorizationUtils.class)) { + authz + .when(() -> AuthorizationUtils.getMetadataObjectLocation(any(), any())) + .thenReturn(ImmutableList.of("/test")); + + Assertions.assertFalse(hook.dropFileset(ident)); + + authz.verify( + () -> AuthorizationUtils.authorizationPluginRemovePrivileges(any(), any(), any()), + Mockito.never()); + } + } + @Test public void testDropAuthorizationPrivilege() { Namespace filesetNs = Namespace.of(metalake, catalog, "schema11212"); diff --git a/core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java b/core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java index 63d2a99f08..a9c3a14fbd 100644 --- a/core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java +++ b/core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java @@ -209,6 +209,28 @@ public class TestSchemaHookDispatcher { "Pre-existing ancestor 'A' must keep its owner; only newly-created schemas are claimed"); } + @Test + public void testDropSchemaKeepsPrivilegesWhenExternalDropReturnsFalse() { + NameIdentifier ident = NameIdentifier.of("test_metalake", "test_catalog", "A:B:C"); + when(mockDispatcher.dropSchema(eq(ident), eq(false))).thenReturn(false); + + try (MockedStatic<AuthorizationUtils> authz = Mockito.mockStatic(AuthorizationUtils.class)) { + authz + .when( + () -> + AuthorizationUtils.getMetadataObjectLocation( + any(NameIdentifier.class), any(Entity.EntityType.class))) + .thenReturn(ImmutableList.of("/test")); + + Assertions.assertFalse(hookDispatcher.dropSchema(ident, false)); + + // Nothing was dropped, so the schema that is still registered keeps its privileges. + authz.verify( + () -> AuthorizationUtils.authorizationPluginRemovePrivileges(any(), any(), any()), + never()); + } + } + @Test public void testDropSchemaRemovesPrivileges() { NameIdentifier ident = NameIdentifier.of("test_metalake", "test_catalog", "A:B:C"); diff --git a/core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java b/core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java index b1a61795a7..befd3a9dd4 100644 --- a/core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java +++ b/core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java @@ -48,6 +48,7 @@ import org.apache.gravitino.rel.expressions.distributions.Distributions; import org.apache.gravitino.rel.expressions.sorts.SortOrder; import org.apache.gravitino.rel.expressions.transforms.Transform; import org.apache.gravitino.rel.indexes.Index; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; import org.mockito.MockedStatic; @@ -81,6 +82,33 @@ public class TestTableHookDispatcher { } } + @Test + public void testDropKeepsPrivilegesWhenExternalDropReturnsFalse() { + TableDispatcher dispatcher = Mockito.mock(TableDispatcher.class); + TableHookDispatcher hook = new TableHookDispatcher(dispatcher, () -> null); + NameIdentifier ident = NameIdentifier.of(METALAKE, CATALOG, "schema", "table"); + // false means the table was renamed or dropped out of band: the registration is kept, so the + // plugin privileges of the entity that is still alive under another name must be kept too. + Mockito.when(dispatcher.dropTable(ident)).thenReturn(false); + Mockito.when(dispatcher.purgeTable(ident)).thenReturn(false); + + try (MockedStatic<AuthorizationUtils> authorizationUtils = + Mockito.mockStatic(AuthorizationUtils.class)) { + authorizationUtils + .when(() -> AuthorizationUtils.getMetadataObjectLocation(ident, Entity.EntityType.TABLE)) + .thenReturn(ImmutableList.of("/test")); + + Assertions.assertFalse(hook.dropTable(ident)); + Assertions.assertFalse(hook.purgeTable(ident)); + + authorizationUtils.verify( + () -> + AuthorizationUtils.authorizationPluginRemovePrivileges( + Mockito.any(), Mockito.any(), Mockito.any()), + Mockito.never()); + } + } + @Test public void testCreateTableSetsOwnerWithNormalizedIdentifier() throws Exception { CatalogManager catalogManager = Mockito.mock(CatalogManager.class); diff --git a/core/src/test/java/org/apache/gravitino/hook/TestTopicHookDispatcher.java b/core/src/test/java/org/apache/gravitino/hook/TestTopicHookDispatcher.java index e4294b2c17..96c851aff7 100644 --- a/core/src/test/java/org/apache/gravitino/hook/TestTopicHookDispatcher.java +++ b/core/src/test/java/org/apache/gravitino/hook/TestTopicHookDispatcher.java @@ -21,6 +21,7 @@ package org.apache.gravitino.hook; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import java.util.Map; import org.apache.commons.lang3.reflect.FieldUtils; @@ -29,6 +30,7 @@ import org.apache.gravitino.MetadataObject; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.Namespace; import org.apache.gravitino.authorization.AccessControlManager; +import org.apache.gravitino.authorization.AuthorizationUtils; import org.apache.gravitino.authorization.Owner; import org.apache.gravitino.authorization.OwnerDispatcher; import org.apache.gravitino.catalog.CatalogManager; @@ -46,6 +48,7 @@ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; +import org.mockito.MockedStatic; import org.mockito.Mockito; public class TestTopicHookDispatcher extends TestOperationDispatcher { @@ -156,6 +159,26 @@ public class TestTopicHookDispatcher extends TestOperationDispatcher { } } + @Test + public void testDropKeepsPrivilegesWhenExternalDropReturnsFalse() { + TopicDispatcher dispatcher = Mockito.mock(TopicDispatcher.class); + TopicHookDispatcher hook = new TopicHookDispatcher(dispatcher); + NameIdentifier ident = NameIdentifier.of(metalake, catalog, "schema", "topic"); + Mockito.when(dispatcher.dropTopic(ident)).thenReturn(false); + + try (MockedStatic<AuthorizationUtils> authz = Mockito.mockStatic(AuthorizationUtils.class)) { + authz + .when(() -> AuthorizationUtils.getMetadataObjectLocation(any(), any())) + .thenReturn(ImmutableList.of("/test")); + + Assertions.assertFalse(hook.dropTopic(ident)); + + authz.verify( + () -> AuthorizationUtils.authorizationPluginRemovePrivileges(any(), any(), any()), + Mockito.never()); + } + } + @Test public void testDropAuthorizationPrivilege() { Namespace topicNs = Namespace.of(metalake, catalog, "schema1123");
