From ca25609624056c4c91ff6a136b1d38154510ccca Mon Sep 17 00:00:00 2001 From: Rory Date: Wed, 9 Oct 2024 20:30:29 +0800 Subject: [PATCH] [#5070] improvement(core): Add check for full name of the metadata object --- .../authorization/AuthorizationUtils.java | 62 ---------- .../org/apache/gravitino/tag/TagManager.java | 55 +++----- .../gravitino/utils/MetadataObjectUtil.java | 79 ++++++++++++ .../apache/gravitino/tag/TestTagManager.java | 6 +- .../server/web/rest/OwnerOperations.java | 2 + .../server/web/rest/PermissionOperations.java | 5 +- .../server/web/rest/RoleOperations.java | 3 +- .../server/web/rest/TestOwnerOperations.java | 48 +++++++ .../server/web/rest/TestRoleOperations.java | 117 ++++++++++++++++-- 9 files changed, 254 insertions(+), 123 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java b/core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java index 66019ee0403..51f5cae6217 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java +++ b/core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java @@ -18,15 +18,12 @@ */ package org.apache.gravitino.authorization; -import static com.google.common.base.Preconditions.checkNotNull; - 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; -import java.util.function.Supplier; import org.apache.gravitino.Catalog; import org.apache.gravitino.Entity; import org.apache.gravitino.EntityStore; @@ -216,58 +213,6 @@ public static boolean needApplyAuthorizationPluginAllCatalogs(SecurableObject se return false; } - // Check every securable object whether exists and is imported. - public static void checkSecurableObject(String metalake, MetadataObject object) { - NameIdentifier identifier = MetadataObjectUtil.toEntityIdent(metalake, object); - - Supplier exceptionToThrowSupplier = - () -> - new NoSuchMetadataObjectException( - "Securable object %s type %s doesn't exist", object.fullName(), object.type()); - - switch (object.type()) { - case METALAKE: - check( - GravitinoEnv.getInstance().metalakeDispatcher().metalakeExists(identifier), - exceptionToThrowSupplier); - break; - - case CATALOG: - check( - GravitinoEnv.getInstance().catalogDispatcher().catalogExists(identifier), - exceptionToThrowSupplier); - break; - - case SCHEMA: - check( - GravitinoEnv.getInstance().schemaDispatcher().schemaExists(identifier), - exceptionToThrowSupplier); - break; - - case FILESET: - check( - GravitinoEnv.getInstance().filesetDispatcher().filesetExists(identifier), - exceptionToThrowSupplier); - break; - - case TABLE: - check( - GravitinoEnv.getInstance().tableDispatcher().tableExists(identifier), - exceptionToThrowSupplier); - break; - - case TOPIC: - check( - GravitinoEnv.getInstance().topicDispatcher().topicExists(identifier), - exceptionToThrowSupplier); - break; - - default: - throw new IllegalArgumentException( - String.format("Doesn't support the type %s", object.type())); - } - } - public static void checkDuplicatedNamePrivilege(Collection privileges) { Set privilegeNameSet = Sets.newHashSet(); for (Privilege privilege : privileges) { @@ -313,13 +258,6 @@ public static void checkPrivilege( } } - private static void check( - final boolean expression, Supplier exceptionToThrowSupplier) { - if (!expression) { - throw checkNotNull(exceptionToThrowSupplier).get(); - } - } - private static void checkCatalogType( NameIdentifier catalogIdent, Catalog.Type type, Privilege privilege) { Catalog catalog = GravitinoEnv.getInstance().catalogDispatcher().loadCatalog(catalogIdent); diff --git a/core/src/main/java/org/apache/gravitino/tag/TagManager.java b/core/src/main/java/org/apache/gravitino/tag/TagManager.java index 040ac9f1953..0ae89f2cf9f 100644 --- a/core/src/main/java/org/apache/gravitino/tag/TagManager.java +++ b/core/src/main/java/org/apache/gravitino/tag/TagManager.java @@ -36,6 +36,7 @@ import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.Namespace; import org.apache.gravitino.exceptions.NoSuchEntityException; +import org.apache.gravitino.exceptions.NoSuchMetadataObjectException; import org.apache.gravitino.exceptions.NoSuchMetalakeException; import org.apache.gravitino.exceptions.NoSuchTagException; import org.apache.gravitino.exceptions.NotFoundException; @@ -240,14 +241,11 @@ public String[] listTagsForMetadataObject(String metalake, MetadataObject metada } public Tag[] listTagsInfoForMetadataObject(String metalake, MetadataObject metadataObject) - throws NotFoundException { + throws NoSuchMetadataObjectException { NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake, metadataObject); Entity.EntityType entityType = MetadataObjectUtil.toEntityType(metadataObject); - if (!checkAndImportEntity(metalake, metadataObject, GravitinoEnv.getInstance())) { - throw new NotFoundException( - "Failed to list tags for metadata object %s due to not found", metadataObject); - } + checkAndImportEntity(metalake, metadataObject, GravitinoEnv.getInstance()); return TreeLockUtils.doWithTreeLock( entityIdent, @@ -258,7 +256,7 @@ public Tag[] listTagsInfoForMetadataObject(String metalake, MetadataObject metad .listAssociatedTagsForMetadataObject(entityIdent, entityType) .toArray(new Tag[0]); } catch (NoSuchEntityException e) { - throw new NotFoundException( + throw new NoSuchMetadataObjectException( e, "Failed to list tags for metadata object %s due to not found", metadataObject); } catch (IOException e) { LOG.error("Failed to list tags for metadata object {}", metadataObject, e); @@ -268,15 +266,12 @@ public Tag[] listTagsInfoForMetadataObject(String metalake, MetadataObject metad } public Tag getTagForMetadataObject(String metalake, MetadataObject metadataObject, String name) - throws NotFoundException { + throws NoSuchMetadataObjectException { NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake, metadataObject); Entity.EntityType entityType = MetadataObjectUtil.toEntityType(metadataObject); NameIdentifier tagIdent = ofTagIdent(metalake, name); - if (!checkAndImportEntity(metalake, metadataObject, GravitinoEnv.getInstance())) { - throw new NotFoundException( - "Failed to get tag for metadata object %s due to not found", metadataObject); - } + checkAndImportEntity(metalake, metadataObject, GravitinoEnv.getInstance()); return TreeLockUtils.doWithTreeLock( entityIdent, @@ -289,7 +284,7 @@ public Tag getTagForMetadataObject(String metalake, MetadataObject metadataObjec throw new NoSuchTagException( e, "Tag %s does not exist for metadata object %s", name, metadataObject); } else { - throw new NotFoundException( + throw new NoSuchMetadataObjectException( e, "Failed to get tag for metadata object %s due to not found", metadataObject); } } catch (IOException e) { @@ -301,20 +296,18 @@ public Tag getTagForMetadataObject(String metalake, MetadataObject metadataObjec public String[] associateTagsForMetadataObject( String metalake, MetadataObject metadataObject, String[] tagsToAdd, String[] tagsToRemove) - throws NotFoundException, TagAlreadyAssociatedException { + throws NoSuchMetadataObjectException, TagAlreadyAssociatedException { Preconditions.checkArgument( !metadataObject.type().equals(MetadataObject.Type.METALAKE) - && !metadataObject.type().equals(MetadataObject.Type.COLUMN), + && !metadataObject.type().equals(MetadataObject.Type.COLUMN) + && !metadataObject.type().equals(MetadataObject.Type.ROLE), "Cannot associate tags for unsupported metadata object type %s", metadataObject.type()); NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake, metadataObject); Entity.EntityType entityType = MetadataObjectUtil.toEntityType(metadataObject); - if (!checkAndImportEntity(metalake, metadataObject, GravitinoEnv.getInstance())) { - throw new NotFoundException( - "Failed to associate tags for metadata object %s due to not found", metadataObject); - } + checkAndImportEntity(metalake, metadataObject, GravitinoEnv.getInstance()); // Remove all the tags that are both set to add and remove Set tagsToAddSet = tagsToAdd == null ? Sets.newHashSet() : Sets.newHashSet(tagsToAdd); @@ -347,7 +340,7 @@ public String[] associateTagsForMetadataObject( .map(Tag::name) .toArray(String[]::new); } catch (NoSuchEntityException e) { - throw new NotFoundException( + throw new NoSuchMetadataObjectException( e, "Failed to associate tags for metadata object %s due to not found", metadataObject); @@ -431,27 +424,7 @@ private TagEntity updateTagEntity(TagEntity tagEntity, TagChange... changes) { // for this entity, with this uid tags can be associated with this entity. // This method should be called out of the tree lock, otherwise it will cause deadlock. @VisibleForTesting - boolean checkAndImportEntity(String metalake, MetadataObject metadataObject, GravitinoEnv env) { - NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake, metadataObject); - Entity.EntityType entityType = MetadataObjectUtil.toEntityType(metadataObject); - - switch (entityType) { - case METALAKE: - return env.metalakeDispatcher().metalakeExists(entityIdent); - case CATALOG: - return env.catalogDispatcher().catalogExists(entityIdent); - case SCHEMA: - return env.schemaDispatcher().schemaExists(entityIdent); - case TABLE: - return env.tableDispatcher().tableExists(entityIdent); - case TOPIC: - return env.topicDispatcher().topicExists(entityIdent); - case FILESET: - return env.filesetDispatcher().filesetExists(entityIdent); - case COLUMN: - default: - throw new IllegalArgumentException( - "Unsupported metadata object type: " + metadataObject.type()); - } + void checkAndImportEntity(String metalake, MetadataObject metadataObject, GravitinoEnv env) { + MetadataObjectUtil.checkMetadataObject(metalake, metadataObject, env); } } diff --git a/core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java b/core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java index 42878ef09f0..b8ec8d339df 100644 --- a/core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java +++ b/core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java @@ -18,16 +18,22 @@ */ package org.apache.gravitino.utils; +import static com.google.common.base.Preconditions.checkNotNull; + import com.google.common.base.Joiner; import com.google.common.base.Preconditions; import com.google.common.collect.BiMap; import com.google.common.collect.ImmutableBiMap; import java.util.Optional; +import java.util.function.Supplier; import org.apache.commons.lang3.StringUtils; import org.apache.gravitino.Entity; +import org.apache.gravitino.GravitinoEnv; import org.apache.gravitino.MetadataObject; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.authorization.AuthorizationUtils; +import org.apache.gravitino.exceptions.NoSuchMetadataObjectException; +import org.apache.gravitino.exceptions.NoSuchRoleException; public class MetadataObjectUtil { @@ -98,4 +104,77 @@ public static NameIdentifier toEntityIdent(String metalakeName, MetadataObject m "Unknown metadata object type: " + metadataObject.type()); } } + + /** + * This method will check if the entity is existed explicitly, internally this check will load the + * entity from underlying sources to entity store if not stored, and will allocate an uid for this + * entity, with this uid tags can be associated with this entity. This method should be called out + * of the tree lock, otherwise it will cause deadlock. + * + * @param metalake The metalake name + * @param object The metadata object + * @param env The Gravitino environment + * @throws NoSuchMetadataObjectException if the metadata object type doesn't exist. + */ + public static void checkMetadataObject(String metalake, MetadataObject object, GravitinoEnv env) { + NameIdentifier identifier = toEntityIdent(metalake, object); + + Supplier exceptionToThrowSupplier = + () -> + new NoSuchMetadataObjectException( + "Metadata object %s type %s doesn't exist", object.fullName(), object.type()); + + switch (object.type()) { + case METALAKE: + NameIdentifierUtil.checkMetalake(identifier); + check(env.metalakeDispatcher().metalakeExists(identifier), exceptionToThrowSupplier); + break; + + case CATALOG: + NameIdentifierUtil.checkCatalog(identifier); + check(env.catalogDispatcher().catalogExists(identifier), exceptionToThrowSupplier); + break; + + case SCHEMA: + NameIdentifierUtil.checkSchema(identifier); + check(env.schemaDispatcher().schemaExists(identifier), exceptionToThrowSupplier); + break; + + case FILESET: + NameIdentifierUtil.checkFileset(identifier); + check(env.filesetDispatcher().filesetExists(identifier), exceptionToThrowSupplier); + break; + + case TABLE: + NameIdentifierUtil.checkTable(identifier); + check(env.tableDispatcher().tableExists(identifier), exceptionToThrowSupplier); + break; + + case TOPIC: + NameIdentifierUtil.checkTopic(identifier); + check(env.topicDispatcher().topicExists(identifier), exceptionToThrowSupplier); + break; + + case ROLE: + AuthorizationUtils.checkRole(identifier); + try { + env.accessControlDispatcher().getRole(metalake, object.fullName()); + } catch (NoSuchRoleException nsr) { + throw checkNotNull(exceptionToThrowSupplier).get(); + } + break; + + case COLUMN: + default: + throw new IllegalArgumentException( + String.format("Doesn't support the type %s", object.type())); + } + } + + private static void check( + final boolean expression, Supplier exceptionToThrowSupplier) { + if (!expression) { + throw checkNotNull(exceptionToThrowSupplier).get(); + } + } } diff --git a/core/src/test/java/org/apache/gravitino/tag/TestTagManager.java b/core/src/test/java/org/apache/gravitino/tag/TestTagManager.java index 3dc298cdbf5..99efc1e45d4 100644 --- a/core/src/test/java/org/apache/gravitino/tag/TestTagManager.java +++ b/core/src/test/java/org/apache/gravitino/tag/TestTagManager.java @@ -31,7 +31,7 @@ import static org.apache.gravitino.Configs.TREE_LOCK_MAX_NODE_IN_MEMORY; import static org.apache.gravitino.Configs.TREE_LOCK_MIN_NODE_IN_MEMORY; import static org.apache.gravitino.Configs.VERSION_RETENTION_COUNT; -import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.spy; import com.google.common.collect.ImmutableMap; @@ -167,9 +167,7 @@ public static void setUp() throws IOException, IllegalAccessException { entityStore.put(table, false /* overwritten */); tagManager = spy(new TagManager(idGenerator, entityStore)); - doReturn(true) - .when(tagManager) - .checkAndImportEntity(Mockito.any(), Mockito.any(), Mockito.any()); + doNothing().when(tagManager).checkAndImportEntity(Mockito.any(), Mockito.any(), Mockito.any()); } @AfterAll diff --git a/server/src/main/java/org/apache/gravitino/server/web/rest/OwnerOperations.java b/server/src/main/java/org/apache/gravitino/server/web/rest/OwnerOperations.java index d4fe66b7f8c..318ccb95e0f 100644 --- a/server/src/main/java/org/apache/gravitino/server/web/rest/OwnerOperations.java +++ b/server/src/main/java/org/apache/gravitino/server/web/rest/OwnerOperations.java @@ -78,6 +78,7 @@ public Response getOwnerForObject( return Utils.doAs( httpRequest, () -> { + MetadataObjectUtil.checkMetadataObject(metalake, object, GravitinoEnv.getInstance()); NameIdentifier ident = MetadataObjectUtil.toEntityIdent(metalake, object); Optional owner = TreeLockUtils.doWithTreeLock( @@ -112,6 +113,7 @@ public Response setOwnerForObject( return Utils.doAs( httpRequest, () -> { + MetadataObjectUtil.checkMetadataObject(metalake, object, GravitinoEnv.getInstance()); NameIdentifier objectIdent = MetadataObjectUtil.toEntityIdent(metalake, object); TreeLockUtils.doWithTreeLock( objectIdent, diff --git a/server/src/main/java/org/apache/gravitino/server/web/rest/PermissionOperations.java b/server/src/main/java/org/apache/gravitino/server/web/rest/PermissionOperations.java index 94ace77aafb..26e59f4f333 100644 --- a/server/src/main/java/org/apache/gravitino/server/web/rest/PermissionOperations.java +++ b/server/src/main/java/org/apache/gravitino/server/web/rest/PermissionOperations.java @@ -50,6 +50,7 @@ import org.apache.gravitino.metrics.MetricNames; import org.apache.gravitino.server.authorization.NameBindings; import org.apache.gravitino.server.web.Utils; +import org.apache.gravitino.utils.MetadataObjectUtil; @NameBindings.AccessControlInterfaces @Path("/metalakes/{metalake}/permissions") @@ -217,7 +218,7 @@ public Response grantPrivilegeToRole( AuthorizationUtils.checkPrivilege(privilegeDTO, object, metalake); } - AuthorizationUtils.checkSecurableObject(metalake, object); + MetadataObjectUtil.checkMetadataObject(metalake, object, GravitinoEnv.getInstance()); return TreeLockUtils.doWithTreeLock( AuthorizationUtils.ofRole(metalake, role), LockType.WRITE, @@ -262,7 +263,7 @@ public Response revokePrivilegeFromRole( AuthorizationUtils.checkPrivilege(privilegeDTO, object, metalake); } - AuthorizationUtils.checkSecurableObject(metalake, object); + MetadataObjectUtil.checkMetadataObject(metalake, object, GravitinoEnv.getInstance()); return TreeLockUtils.doWithTreeLock( AuthorizationUtils.ofRole(metalake, role), LockType.WRITE, 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 e20f6b4510a..389cf63771b 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 @@ -55,6 +55,7 @@ import org.apache.gravitino.metrics.MetricNames; import org.apache.gravitino.server.authorization.NameBindings; import org.apache.gravitino.server.web.Utils; +import org.apache.gravitino.utils.MetadataObjectUtil; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -142,7 +143,7 @@ public Response createRole(@PathParam("metalake") String metalake, RoleCreateReq for (Privilege privilege : object.privileges()) { AuthorizationUtils.checkPrivilege((PrivilegeDTO) privilege, object, metalake); } - AuthorizationUtils.checkSecurableObject(metalake, object); + MetadataObjectUtil.checkMetadataObject(metalake, object, GravitinoEnv.getInstance()); } List securableObjects = diff --git a/server/src/test/java/org/apache/gravitino/server/web/rest/TestOwnerOperations.java b/server/src/test/java/org/apache/gravitino/server/web/rest/TestOwnerOperations.java index 2b0b3ff4aa3..574197654f3 100644 --- a/server/src/test/java/org/apache/gravitino/server/web/rest/TestOwnerOperations.java +++ b/server/src/test/java/org/apache/gravitino/server/web/rest/TestOwnerOperations.java @@ -36,17 +36,25 @@ import org.apache.commons.lang3.reflect.FieldUtils; import org.apache.gravitino.Config; import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.MetadataObject; +import org.apache.gravitino.MetadataObjects; +import org.apache.gravitino.authorization.AccessControlDispatcher; import org.apache.gravitino.authorization.Owner; import org.apache.gravitino.authorization.OwnerManager; +import org.apache.gravitino.authorization.Role; import org.apache.gravitino.dto.authorization.OwnerDTO; import org.apache.gravitino.dto.requests.OwnerSetRequest; import org.apache.gravitino.dto.responses.ErrorConstants; import org.apache.gravitino.dto.responses.ErrorResponse; import org.apache.gravitino.dto.responses.OwnerResponse; import org.apache.gravitino.dto.responses.SetResponse; +import org.apache.gravitino.exceptions.NoSuchMetadataObjectException; +import org.apache.gravitino.exceptions.NoSuchRoleException; import org.apache.gravitino.exceptions.NotFoundException; import org.apache.gravitino.lock.LockManager; +import org.apache.gravitino.metalake.MetalakeDispatcher; import org.apache.gravitino.rest.RESTUtils; +import org.apache.gravitino.utils.MetadataObjectUtil; import org.glassfish.hk2.utilities.binding.AbstractBinder; import org.glassfish.jersey.server.ResourceConfig; import org.glassfish.jersey.test.JerseyTest; @@ -58,6 +66,9 @@ class TestOwnerOperations extends JerseyTest { private static final OwnerManager manager = mock(OwnerManager.class); + private static final MetalakeDispatcher metalakeDispatcher = mock(MetalakeDispatcher.class); + private static final AccessControlDispatcher accessControlDispatcher = + mock(AccessControlDispatcher.class); private static class MockServletRequestFactory extends ServletRequestFactoryBase { @Override @@ -76,6 +87,10 @@ public static void setup() throws IllegalAccessException { Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerManager", manager, true); + FieldUtils.writeField( + GravitinoEnv.getInstance(), "metalakeDispatcher", metalakeDispatcher, true); + FieldUtils.writeField( + GravitinoEnv.getInstance(), "accessControlDispatcher", accessControlDispatcher, true); } @Override @@ -116,6 +131,7 @@ public Type type() { }; when(manager.getOwner(any(), any())).thenReturn(Optional.of(owner)); + when(metalakeDispatcher.metalakeExists(any())).thenReturn(true); Response resp = target("/metalakes/metalake1/owners/metalake/metalake1") @@ -172,10 +188,20 @@ public Type type() { ErrorResponse errorResponse2 = resp3.readEntity(ErrorResponse.class); Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE, errorResponse2.getCode()); + + // Test to throw IllegalNamespaceException + Response resp4 = + target("/metalakes/metalake1/owners/catalog/metalake1.catalog1") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .get(); + ErrorResponse errorResponse3 = resp4.readEntity(ErrorResponse.class); + Assertions.assertEquals(ErrorConstants.ILLEGAL_ARGUMENTS_CODE, errorResponse3.getCode()); } @Test void testSetOwnerForObject() { + when(metalakeDispatcher.metalakeExists(any())).thenReturn(true); OwnerSetRequest request = new OwnerSetRequest("test", Owner.Type.USER); Response resp = target("/metalakes/metalake1/owners/metalake/metalake1") @@ -216,5 +242,27 @@ void testSetOwnerForObject() { ErrorResponse errorResponse2 = resp3.readEntity(ErrorResponse.class); Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE, errorResponse2.getCode()); + + // Test to throw IllegalNamespaceException + Response resp4 = + target("/metalakes/metalake1/owners/catalog/metalake1.catalog1") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .put(Entity.entity(request, MediaType.APPLICATION_JSON_TYPE)); + ErrorResponse errorResponse3 = resp4.readEntity(ErrorResponse.class); + Assertions.assertEquals(ErrorConstants.ILLEGAL_ARGUMENTS_CODE, errorResponse3.getCode()); + } + + @Test + public void testRoleObject() { + MetadataObject role = MetadataObjects.of(null, "role", MetadataObject.Type.ROLE); + when(accessControlDispatcher.getRole(any(), any())).thenReturn(mock(Role.class)); + Assertions.assertDoesNotThrow( + () -> MetadataObjectUtil.checkMetadataObject("metalake", role, GravitinoEnv.getInstance())); + + doThrow(new NoSuchRoleException("test")).when(accessControlDispatcher).getRole(any(), any()); + Assertions.assertThrows( + NoSuchMetadataObjectException.class, + () -> MetadataObjectUtil.checkMetadataObject("metalake", role, GravitinoEnv.getInstance())); } } 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 265fb607353..b7d5e5968ac 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 @@ -31,6 +31,8 @@ import java.io.IOException; import java.time.Instant; import java.util.Collections; +import java.util.concurrent.atomic.AtomicReference; +import javax.annotation.Nullable; import javax.servlet.http.HttpServletRequest; import javax.ws.rs.client.Entity; import javax.ws.rs.core.Application; @@ -39,8 +41,8 @@ import org.apache.commons.lang3.reflect.FieldUtils; import org.apache.gravitino.Config; import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.MetadataObject; import org.apache.gravitino.authorization.AccessControlManager; -import org.apache.gravitino.authorization.AuthorizationUtils; import org.apache.gravitino.authorization.Privileges; import org.apache.gravitino.authorization.Role; import org.apache.gravitino.authorization.SecurableObject; @@ -59,6 +61,7 @@ import org.apache.gravitino.dto.responses.NameListResponse; import org.apache.gravitino.dto.responses.RoleResponse; import org.apache.gravitino.dto.util.DTOConverters; +import org.apache.gravitino.exceptions.IllegalNamespaceException; import org.apache.gravitino.exceptions.IllegalPrivilegeException; import org.apache.gravitino.exceptions.NoSuchMetadataObjectException; import org.apache.gravitino.exceptions.NoSuchMetalakeException; @@ -69,6 +72,7 @@ import org.apache.gravitino.meta.RoleEntity; import org.apache.gravitino.metalake.MetalakeDispatcher; import org.apache.gravitino.rest.RESTUtils; +import org.apache.gravitino.utils.MetadataObjectUtil; import org.glassfish.hk2.utilities.binding.AbstractBinder; import org.glassfish.jersey.server.ResourceConfig; import org.glassfish.jersey.test.JerseyTest; @@ -105,8 +109,6 @@ public static void setup() throws IllegalAccessException { Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); FieldUtils.writeField(GravitinoEnv.getInstance(), "accessControlDispatcher", manager, true); - FieldUtils.writeField( - GravitinoEnv.getInstance(), "metalakeDispatcher", metalakeDispatcher, true); FieldUtils.writeField( GravitinoEnv.getInstance(), "metalakeDispatcher", metalakeDispatcher, true); FieldUtils.writeField(GravitinoEnv.getInstance(), "catalogDispatcher", catalogDispatcher, true); @@ -439,11 +441,15 @@ public void testCheckSecurableObjects() { SecurableObjects.ofCatalog("catalog", Lists.newArrayList(Privileges.UseCatalog.allow())); when(catalogDispatcher.catalogExists(any())).thenReturn(true); Assertions.assertDoesNotThrow( - () -> AuthorizationUtils.checkSecurableObject("metalake", DTOConverters.toDTO(catalog))); + () -> + MetadataObjectUtil.checkMetadataObject( + "metalake", DTOConverters.toDTO(catalog), GravitinoEnv.getInstance())); when(catalogDispatcher.catalogExists(any())).thenReturn(false); Assertions.assertThrows( NoSuchMetadataObjectException.class, - () -> AuthorizationUtils.checkSecurableObject("metalake", DTOConverters.toDTO(catalog))); + () -> + MetadataObjectUtil.checkMetadataObject( + "metalake", DTOConverters.toDTO(catalog), GravitinoEnv.getInstance())); // check the schema SecurableObject schema = @@ -451,11 +457,15 @@ public void testCheckSecurableObjects() { catalog, "schema", Lists.newArrayList(Privileges.UseSchema.allow())); when(schemaDispatcher.schemaExists(any())).thenReturn(true); Assertions.assertDoesNotThrow( - () -> AuthorizationUtils.checkSecurableObject("metalake", DTOConverters.toDTO(schema))); + () -> + MetadataObjectUtil.checkMetadataObject( + "metalake", DTOConverters.toDTO(schema), GravitinoEnv.getInstance())); when(schemaDispatcher.schemaExists(any())).thenReturn(false); Assertions.assertThrows( NoSuchMetadataObjectException.class, - () -> AuthorizationUtils.checkSecurableObject("metalake", DTOConverters.toDTO(schema))); + () -> + MetadataObjectUtil.checkMetadataObject( + "metalake", DTOConverters.toDTO(schema), GravitinoEnv.getInstance())); // check the table SecurableObject table = @@ -463,11 +473,15 @@ public void testCheckSecurableObjects() { schema, "table", Lists.newArrayList(Privileges.SelectTable.allow())); when(tableDispatcher.tableExists(any())).thenReturn(true); Assertions.assertDoesNotThrow( - () -> AuthorizationUtils.checkSecurableObject("metalake", DTOConverters.toDTO(table))); + () -> + MetadataObjectUtil.checkMetadataObject( + "metalake", DTOConverters.toDTO(table), GravitinoEnv.getInstance())); when(tableDispatcher.tableExists(any())).thenReturn(false); Assertions.assertThrows( NoSuchMetadataObjectException.class, - () -> AuthorizationUtils.checkSecurableObject("metalake", DTOConverters.toDTO(table))); + () -> + MetadataObjectUtil.checkMetadataObject( + "metalake", DTOConverters.toDTO(table), GravitinoEnv.getInstance())); // check the topic SecurableObject topic = @@ -475,11 +489,15 @@ public void testCheckSecurableObjects() { schema, "topic", Lists.newArrayList(Privileges.ConsumeTopic.allow())); when(topicDispatcher.topicExists(any())).thenReturn(true); Assertions.assertDoesNotThrow( - () -> AuthorizationUtils.checkSecurableObject("metalake", DTOConverters.toDTO(topic))); + () -> + MetadataObjectUtil.checkMetadataObject( + "metalake", DTOConverters.toDTO(topic), GravitinoEnv.getInstance())); when(topicDispatcher.topicExists(any())).thenReturn(false); Assertions.assertThrows( NoSuchMetadataObjectException.class, - () -> AuthorizationUtils.checkSecurableObject("metalake", DTOConverters.toDTO(topic))); + () -> + MetadataObjectUtil.checkMetadataObject( + "metalake", DTOConverters.toDTO(topic), GravitinoEnv.getInstance())); // check the fileset SecurableObject fileset = @@ -487,11 +505,84 @@ public void testCheckSecurableObjects() { schema, "fileset", Lists.newArrayList(Privileges.ReadFileset.allow())); when(filesetDispatcher.filesetExists(any())).thenReturn(true); Assertions.assertDoesNotThrow( - () -> AuthorizationUtils.checkSecurableObject("metalake", DTOConverters.toDTO(fileset))); + () -> + MetadataObjectUtil.checkMetadataObject( + "metalake", DTOConverters.toDTO(fileset), GravitinoEnv.getInstance())); when(filesetDispatcher.filesetExists(any())).thenReturn(false); Assertions.assertThrows( NoSuchMetadataObjectException.class, - () -> AuthorizationUtils.checkSecurableObject("metalake", DTOConverters.toDTO(fileset))); + () -> + MetadataObjectUtil.checkMetadataObject( + "metalake", DTOConverters.toDTO(fileset), GravitinoEnv.getInstance())); + + AtomicReference wrongParent = new AtomicReference<>(); + AtomicReference wrongName = new AtomicReference<>(); + AtomicReference wrongType = new AtomicReference<>(); + wrongParent.set("catalog1"); + wrongName.set("schema1"); + + MetadataObject wrongMetadataObject = + new MetadataObject() { + @Nullable + @Override + public String parent() { + return wrongParent.get(); + } + + @Override + public String name() { + return wrongName.get(); + } + + @Override + public Type type() { + return wrongType.get(); + } + }; + + // Test catalog object + wrongType.set(MetadataObject.Type.CATALOG); + Assertions.assertThrows( + IllegalNamespaceException.class, + () -> + MetadataObjectUtil.checkMetadataObject( + "metalake", wrongMetadataObject, GravitinoEnv.getInstance())); + + // Test schema object + wrongType.set(MetadataObject.Type.CATALOG); + wrongParent.set("catalog1.schema1"); + Assertions.assertThrows( + IllegalNamespaceException.class, + () -> + MetadataObjectUtil.checkMetadataObject( + "metalake", wrongMetadataObject, GravitinoEnv.getInstance())); + + // Test table object + wrongType.set(MetadataObject.Type.TABLE); + wrongParent.set("catalog1"); + Assertions.assertThrows( + IllegalNamespaceException.class, + () -> + MetadataObjectUtil.checkMetadataObject( + "metalake", wrongMetadataObject, GravitinoEnv.getInstance())); + + // Test fileset object + wrongType.set(MetadataObject.Type.FILESET); + wrongParent.set("catalog1"); + Assertions.assertThrows( + IllegalNamespaceException.class, + () -> + MetadataObjectUtil.checkMetadataObject( + "metalake", wrongMetadataObject, GravitinoEnv.getInstance())); + + // Test topic object + wrongType.set(MetadataObject.Type.TOPIC); + wrongParent.set("catalog1"); + Assertions.assertThrows( + IllegalNamespaceException.class, + () -> + MetadataObjectUtil.checkMetadataObject( + "metalake", wrongMetadataObject, GravitinoEnv.getInstance())); } @Test