From 7bd2f4ee6835e036b944dd6ca89ff897a67cc879 Mon Sep 17 00:00:00 2001 From: yangliwei Date: Tue, 23 Apr 2024 19:51:41 +0800 Subject: [PATCH 1/6] fix: user, group and role namespace validation --- .../datastrato/gravitino/NameIdentifier.java | 33 ++++++++++++++++ .../com/datastrato/gravitino/Namespace.java | 39 +++++++++++++++++++ .../relational/service/GroupMetaService.java | 26 +++---------- .../relational/service/RoleMetaService.java | 20 +++------- .../relational/service/UserMetaService.java | 26 +++---------- 5 files changed, 89 insertions(+), 55 deletions(-) diff --git a/api/src/main/java/com/datastrato/gravitino/NameIdentifier.java b/api/src/main/java/com/datastrato/gravitino/NameIdentifier.java index b3c40bbdbcf..7b9dcde5f0e 100644 --- a/api/src/main/java/com/datastrato/gravitino/NameIdentifier.java +++ b/api/src/main/java/com/datastrato/gravitino/NameIdentifier.java @@ -196,6 +196,39 @@ public static void checkTopic(NameIdentifier ident) { Namespace.checkTopic(ident.namespace); } + /** + * Check the given {@link NameIdentifier} is a user identifier. Throw an {@link + * IllegalNameIdentifierException} if it's not. + * + * @param ident The user {@link NameIdentifier} to check. + */ + public static void checkUser(NameIdentifier ident) { + check(ident != null, "User identifier must not be null"); + Namespace.checkUser(ident.namespace); + } + + /** + * Check the given {@link NameIdentifier} is a group identifier. Throw an {@link + * IllegalNameIdentifierException} if it's not. + * + * @param ident The group {@link NameIdentifier} to check. + */ + public static void checkGroup(NameIdentifier ident) { + check(ident != null, "Group identifier must not be null"); + Namespace.checkGroup(ident.namespace); + } + + /** + * Check the given {@link NameIdentifier} is a role identifier. Throw an {@link + * IllegalNameIdentifierException} if it's not. + * + * @param ident The role {@link NameIdentifier} to check. + */ + public static void checkRole(NameIdentifier ident) { + check(ident != null, "Role identifier must not be null"); + Namespace.checkRole(ident.namespace); + } + /** * Create a {@link NameIdentifier} from the given identifier string. * diff --git a/api/src/main/java/com/datastrato/gravitino/Namespace.java b/api/src/main/java/com/datastrato/gravitino/Namespace.java index ae469ff3d4c..827147e60ec 100644 --- a/api/src/main/java/com/datastrato/gravitino/Namespace.java +++ b/api/src/main/java/com/datastrato/gravitino/Namespace.java @@ -195,6 +195,45 @@ public static void checkTopic(Namespace namespace) { namespace); } + /** + * Check if the given user namespace is legal, throw an {@link IllegalNamespaceException} if it's + * illegal. + * + * @param namespace The user namespace + */ + public static void checkUser(Namespace namespace) { + check( + namespace != null && namespace.length() == 3, + "User namespace must be non-null and have 3 levels, the input namespace is %s", + namespace); + } + + /** + * Check if the given group namespace is legal, throw an {@link IllegalNamespaceException} if it's + * illegal. + * + * @param namespace The group namespace + */ + public static void checkGroup(Namespace namespace) { + check( + namespace != null && namespace.length() == 3, + "Group namespace must be non-null and have 3 levels, the input namespace is %s", + namespace); + } + + /** + * Check if the given role namespace is legal, throw an {@link IllegalNamespaceException} if it's + * illegal. + * + * @param namespace The role namespace + */ + public static void checkRole(Namespace namespace) { + check( + namespace != null && namespace.length() == 3, + "Role namespace must be non-null and have 3 levels, the input namespace is %s", + namespace); + } + private Namespace(String[] levels) { this.levels = levels; } diff --git a/core/src/main/java/com/datastrato/gravitino/storage/relational/service/GroupMetaService.java b/core/src/main/java/com/datastrato/gravitino/storage/relational/service/GroupMetaService.java index a084518203a..9d19312568d 100644 --- a/core/src/main/java/com/datastrato/gravitino/storage/relational/service/GroupMetaService.java +++ b/core/src/main/java/com/datastrato/gravitino/storage/relational/service/GroupMetaService.java @@ -67,11 +67,8 @@ private Long getGroupIdByMetalakeIdAndName(Long metalakeId, String groupName) { } public GroupEntity getGroupByIdentifier(NameIdentifier identifier) { - Preconditions.checkArgument( - identifier != null - && !identifier.namespace().isEmpty() - && identifier.namespace().levels().length == 3, - "The identifier should not be null and should have three level."); + NameIdentifier.checkGroup(identifier); + Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0)); GroupPO groupPO = getGroupPOByMetalakeIdAndName(metalakeId, identifier.name()); @@ -82,11 +79,7 @@ public GroupEntity getGroupByIdentifier(NameIdentifier identifier) { public void insertGroup(GroupEntity groupEntity, boolean overwritten) { try { - Preconditions.checkArgument( - groupEntity.namespace() != null - && !groupEntity.namespace().isEmpty() - && groupEntity.namespace().levels().length == 3, - "The namespace of groupEntity should not be null and should have three level."); + NameIdentifier.checkGroup(groupEntity.nameIdentifier()); Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(groupEntity.namespace().level(0)); @@ -130,11 +123,8 @@ public void insertGroup(GroupEntity groupEntity, boolean overwritten) { } public boolean deleteGroup(NameIdentifier identifier) { - Preconditions.checkArgument( - identifier != null - && !identifier.namespace().isEmpty() - && identifier.namespace().levels().length == 3, - "The identifier should not be null and should have three level."); + NameIdentifier.checkGroup(identifier); + Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0)); Long groupId = getGroupIdByMetalakeIdAndName(metalakeId, identifier.name()); @@ -152,11 +142,7 @@ public boolean deleteGroup(NameIdentifier identifier) { public GroupEntity updateGroup( NameIdentifier identifier, Function updater) { - Preconditions.checkArgument( - identifier != null - && !identifier.namespace().isEmpty() - && identifier.namespace().levels().length == 3, - "The identifier should not be null and should have three level."); + NameIdentifier.checkGroup(identifier); Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0)); diff --git a/core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java b/core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java index 7d82f979bc9..370bea6e488 100644 --- a/core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java +++ b/core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java @@ -70,11 +70,7 @@ public List listRolesByGroupId(Long groupId) { public void insertRole(RoleEntity roleEntity, boolean overwritten) { try { - Preconditions.checkArgument( - roleEntity.namespace() != null - && !roleEntity.namespace().isEmpty() - && roleEntity.namespace().levels().length == 3, - "The namespace of RoleEntity must have three levels."); + NameIdentifier.checkRole(roleEntity.nameIdentifier()); Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(roleEntity.namespace().level(0)); @@ -99,11 +95,8 @@ public void insertRole(RoleEntity roleEntity, boolean overwritten) { } public RoleEntity getRoleByIdentifier(NameIdentifier identifier) { - Preconditions.checkArgument( - identifier != null - && !identifier.namespace().isEmpty() - && identifier.namespace().levels().length == 3, - "The identifier of Role must have three levels."); + NameIdentifier.checkRole(identifier); + Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0)); RolePO rolePO = getRolePOByMetalakeIdAndName(metalakeId, identifier.name()); @@ -112,11 +105,8 @@ public RoleEntity getRoleByIdentifier(NameIdentifier identifier) { } public boolean deleteRole(NameIdentifier identifier) { - Preconditions.checkArgument( - identifier != null - && !identifier.namespace().isEmpty() - && identifier.namespace().levels().length == 3, - "The identifier of Role must have three levels."); + NameIdentifier.checkRole(identifier); + Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0)); Long roleId = getRoleIdByMetalakeIdAndName(metalakeId, identifier.name()); diff --git a/core/src/main/java/com/datastrato/gravitino/storage/relational/service/UserMetaService.java b/core/src/main/java/com/datastrato/gravitino/storage/relational/service/UserMetaService.java index 5ebe1a78171..378bb807132 100644 --- a/core/src/main/java/com/datastrato/gravitino/storage/relational/service/UserMetaService.java +++ b/core/src/main/java/com/datastrato/gravitino/storage/relational/service/UserMetaService.java @@ -67,11 +67,8 @@ private Long getUserIdByMetalakeIdAndName(Long metalakeId, String userName) { } public UserEntity getUserByIdentifier(NameIdentifier identifier) { - Preconditions.checkArgument( - identifier != null - && !identifier.namespace().isEmpty() - && identifier.namespace().levels().length == 3, - "The identifier should not be null and should have three level."); + NameIdentifier.checkUser(identifier); + Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0)); UserPO userPO = getUserPOByMetalakeIdAndName(metalakeId, identifier.name()); @@ -82,11 +79,7 @@ public UserEntity getUserByIdentifier(NameIdentifier identifier) { public void insertUser(UserEntity userEntity, boolean overwritten) { try { - Preconditions.checkArgument( - userEntity.namespace() != null - && !userEntity.namespace().isEmpty() - && userEntity.namespace().levels().length == 3, - "The identifier should not be null and should have three level."); + NameIdentifier.checkUser(userEntity.nameIdentifier()); Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(userEntity.namespace().level(0)); @@ -130,11 +123,8 @@ public void insertUser(UserEntity userEntity, boolean overwritten) { } public boolean deleteUser(NameIdentifier identifier) { - Preconditions.checkArgument( - identifier != null - && !identifier.namespace().isEmpty() - && identifier.namespace().levels().length == 3, - "The identifier should not be null and should have three level."); + NameIdentifier.checkUser(identifier); + Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0)); Long userId = getUserIdByMetalakeIdAndName(metalakeId, identifier.name()); @@ -151,11 +141,7 @@ public boolean deleteUser(NameIdentifier identifier) { public UserEntity updateUser( NameIdentifier identifier, Function updater) { - Preconditions.checkArgument( - identifier != null - && !identifier.namespace().isEmpty() - && identifier.namespace().levels().length == 3, - "The identifier should not be null and should have three level."); + NameIdentifier.checkUser(identifier); Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0)); From 685321de220717e8742fed8413a16b1368a9a395 Mon Sep 17 00:00:00 2001 From: yangliwei Date: Tue, 23 Apr 2024 20:11:57 +0800 Subject: [PATCH 2/6] fix: test --- .../com/datastrato/gravitino/Namespace.java | 18 ++++++++-------- .../gravitino/TestNameIdentifier.java | 18 ++++++++++++++++ .../datastrato/gravitino/TestNamespace.java | 21 +++++++++++++++++++ .../relational/service/RoleMetaService.java | 1 - 4 files changed, 48 insertions(+), 10 deletions(-) diff --git a/api/src/main/java/com/datastrato/gravitino/Namespace.java b/api/src/main/java/com/datastrato/gravitino/Namespace.java index 827147e60ec..23014c8a96a 100644 --- a/api/src/main/java/com/datastrato/gravitino/Namespace.java +++ b/api/src/main/java/com/datastrato/gravitino/Namespace.java @@ -203,9 +203,9 @@ public static void checkTopic(Namespace namespace) { */ public static void checkUser(Namespace namespace) { check( - namespace != null && namespace.length() == 3, - "User namespace must be non-null and have 3 levels, the input namespace is %s", - namespace); + namespace != null && namespace.length() == 3, + "User namespace must be non-null and have 3 levels, the input namespace is %s", + namespace); } /** @@ -216,9 +216,9 @@ public static void checkUser(Namespace namespace) { */ public static void checkGroup(Namespace namespace) { check( - namespace != null && namespace.length() == 3, - "Group namespace must be non-null and have 3 levels, the input namespace is %s", - namespace); + namespace != null && namespace.length() == 3, + "Group namespace must be non-null and have 3 levels, the input namespace is %s", + namespace); } /** @@ -229,9 +229,9 @@ public static void checkGroup(Namespace namespace) { */ public static void checkRole(Namespace namespace) { check( - namespace != null && namespace.length() == 3, - "Role namespace must be non-null and have 3 levels, the input namespace is %s", - namespace); + namespace != null && namespace.length() == 3, + "Role namespace must be non-null and have 3 levels, the input namespace is %s", + namespace); } private Namespace(String[] levels) { diff --git a/api/src/test/java/com/datastrato/gravitino/TestNameIdentifier.java b/api/src/test/java/com/datastrato/gravitino/TestNameIdentifier.java index aefb79bf8eb..d7970463aaa 100644 --- a/api/src/test/java/com/datastrato/gravitino/TestNameIdentifier.java +++ b/api/src/test/java/com/datastrato/gravitino/TestNameIdentifier.java @@ -121,5 +121,23 @@ public void testCheckNameIdentifier() { Throwable excep3 = assertThrows(IllegalNamespaceException.class, () -> NameIdentifier.checkTable(abc)); assertTrue(excep3.getMessage().contains("Table namespace must be non-null and have 3 levels")); + + // test user + assertThrows(IllegalNameIdentifierException.class, () -> NameIdentifier.checkUser(null)); + Throwable excep4 = + assertThrows(IllegalNamespaceException.class, () -> NameIdentifier.checkUser(abc)); + assertTrue(excep4.getMessage().contains("User namespace must be non-null and have 3 levels")); + + // test group + assertThrows(IllegalNameIdentifierException.class, () -> NameIdentifier.checkGroup(null)); + Throwable excep5 = + assertThrows(IllegalNamespaceException.class, () -> NameIdentifier.checkGroup(abc)); + assertTrue(excep5.getMessage().contains("Group namespace must be non-null and have 3 levels")); + + // test role + assertThrows(IllegalNameIdentifierException.class, () -> NameIdentifier.checkRole(null)); + Throwable excep6 = + assertThrows(IllegalNamespaceException.class, () -> NameIdentifier.checkRole(abc)); + assertTrue(excep6.getMessage().contains("Role namespace must be non-null and have 3 levels")); } } diff --git a/api/src/test/java/com/datastrato/gravitino/TestNamespace.java b/api/src/test/java/com/datastrato/gravitino/TestNamespace.java index 8b12b75c22f..113f23fbec7 100644 --- a/api/src/test/java/com/datastrato/gravitino/TestNamespace.java +++ b/api/src/test/java/com/datastrato/gravitino/TestNamespace.java @@ -67,5 +67,26 @@ public void testCheckNamespace() { Assertions.assertThrows(IllegalNamespaceException.class, () -> Namespace.checkTable(abcd)); Assertions.assertTrue( excep3.getMessage().contains("Table namespace must be non-null and have 3 levels")); + + // Test user + Assertions.assertThrows(IllegalNamespaceException.class, () -> Namespace.checkUser(null)); + Throwable excep4 = + Assertions.assertThrows(IllegalNamespaceException.class, () -> Namespace.checkUser(abcd)); + Assertions.assertTrue( + excep4.getMessage().contains("User namespace must be non-null and have 3 levels")); + + // Test group + Assertions.assertThrows(IllegalNamespaceException.class, () -> Namespace.checkGroup(null)); + Throwable excep5 = + Assertions.assertThrows(IllegalNamespaceException.class, () -> Namespace.checkGroup(abcd)); + Assertions.assertTrue( + excep5.getMessage().contains("Group namespace must be non-null and have 3 levels")); + + // Test role + Assertions.assertThrows(IllegalNamespaceException.class, () -> Namespace.checkRole(null)); + Throwable excep6 = + Assertions.assertThrows(IllegalNamespaceException.class, () -> Namespace.checkRole(abcd)); + Assertions.assertTrue( + excep6.getMessage().contains("Role namespace must be non-null and have 3 levels")); } } diff --git a/core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java b/core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java index 370bea6e488..b0ad60740d3 100644 --- a/core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java +++ b/core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java @@ -15,7 +15,6 @@ import com.datastrato.gravitino.storage.relational.utils.ExceptionUtils; import com.datastrato.gravitino.storage.relational.utils.POConverters; import com.datastrato.gravitino.storage.relational.utils.SessionUtils; -import com.google.common.base.Preconditions; import java.util.List; /** The service class for role metadata. It provides the basic database operations for role. */ From b103d386f7da246194ccf557c4d7c482167e8a23 Mon Sep 17 00:00:00 2001 From: yangliwei Date: Wed, 24 Apr 2024 10:08:23 +0800 Subject: [PATCH 3/6] fix: optimize --- .../datastrato/gravitino/NameIdentifier.java | 33 ----- .../com/datastrato/gravitino/Namespace.java | 39 ------ .../gravitino/TestNameIdentifier.java | 18 --- .../datastrato/gravitino/TestNamespace.java | 21 ---- .../authorization/AuthorizationUtils.java | 46 +++++++ .../relational/service/GroupMetaService.java | 9 +- .../relational/service/RoleMetaService.java | 7 +- .../relational/service/UserMetaService.java | 9 +- .../authorization/TestAuthorizationUtils.java | 115 ++++++++++++++++++ 9 files changed, 175 insertions(+), 122 deletions(-) create mode 100644 core/src/test/java/com/datastrato/gravitino/authorization/TestAuthorizationUtils.java diff --git a/api/src/main/java/com/datastrato/gravitino/NameIdentifier.java b/api/src/main/java/com/datastrato/gravitino/NameIdentifier.java index 7b9dcde5f0e..b3c40bbdbcf 100644 --- a/api/src/main/java/com/datastrato/gravitino/NameIdentifier.java +++ b/api/src/main/java/com/datastrato/gravitino/NameIdentifier.java @@ -196,39 +196,6 @@ public static void checkTopic(NameIdentifier ident) { Namespace.checkTopic(ident.namespace); } - /** - * Check the given {@link NameIdentifier} is a user identifier. Throw an {@link - * IllegalNameIdentifierException} if it's not. - * - * @param ident The user {@link NameIdentifier} to check. - */ - public static void checkUser(NameIdentifier ident) { - check(ident != null, "User identifier must not be null"); - Namespace.checkUser(ident.namespace); - } - - /** - * Check the given {@link NameIdentifier} is a group identifier. Throw an {@link - * IllegalNameIdentifierException} if it's not. - * - * @param ident The group {@link NameIdentifier} to check. - */ - public static void checkGroup(NameIdentifier ident) { - check(ident != null, "Group identifier must not be null"); - Namespace.checkGroup(ident.namespace); - } - - /** - * Check the given {@link NameIdentifier} is a role identifier. Throw an {@link - * IllegalNameIdentifierException} if it's not. - * - * @param ident The role {@link NameIdentifier} to check. - */ - public static void checkRole(NameIdentifier ident) { - check(ident != null, "Role identifier must not be null"); - Namespace.checkRole(ident.namespace); - } - /** * Create a {@link NameIdentifier} from the given identifier string. * diff --git a/api/src/main/java/com/datastrato/gravitino/Namespace.java b/api/src/main/java/com/datastrato/gravitino/Namespace.java index 23014c8a96a..ae469ff3d4c 100644 --- a/api/src/main/java/com/datastrato/gravitino/Namespace.java +++ b/api/src/main/java/com/datastrato/gravitino/Namespace.java @@ -195,45 +195,6 @@ public static void checkTopic(Namespace namespace) { namespace); } - /** - * Check if the given user namespace is legal, throw an {@link IllegalNamespaceException} if it's - * illegal. - * - * @param namespace The user namespace - */ - public static void checkUser(Namespace namespace) { - check( - namespace != null && namespace.length() == 3, - "User namespace must be non-null and have 3 levels, the input namespace is %s", - namespace); - } - - /** - * Check if the given group namespace is legal, throw an {@link IllegalNamespaceException} if it's - * illegal. - * - * @param namespace The group namespace - */ - public static void checkGroup(Namespace namespace) { - check( - namespace != null && namespace.length() == 3, - "Group namespace must be non-null and have 3 levels, the input namespace is %s", - namespace); - } - - /** - * Check if the given role namespace is legal, throw an {@link IllegalNamespaceException} if it's - * illegal. - * - * @param namespace The role namespace - */ - public static void checkRole(Namespace namespace) { - check( - namespace != null && namespace.length() == 3, - "Role namespace must be non-null and have 3 levels, the input namespace is %s", - namespace); - } - private Namespace(String[] levels) { this.levels = levels; } diff --git a/api/src/test/java/com/datastrato/gravitino/TestNameIdentifier.java b/api/src/test/java/com/datastrato/gravitino/TestNameIdentifier.java index d7970463aaa..aefb79bf8eb 100644 --- a/api/src/test/java/com/datastrato/gravitino/TestNameIdentifier.java +++ b/api/src/test/java/com/datastrato/gravitino/TestNameIdentifier.java @@ -121,23 +121,5 @@ public void testCheckNameIdentifier() { Throwable excep3 = assertThrows(IllegalNamespaceException.class, () -> NameIdentifier.checkTable(abc)); assertTrue(excep3.getMessage().contains("Table namespace must be non-null and have 3 levels")); - - // test user - assertThrows(IllegalNameIdentifierException.class, () -> NameIdentifier.checkUser(null)); - Throwable excep4 = - assertThrows(IllegalNamespaceException.class, () -> NameIdentifier.checkUser(abc)); - assertTrue(excep4.getMessage().contains("User namespace must be non-null and have 3 levels")); - - // test group - assertThrows(IllegalNameIdentifierException.class, () -> NameIdentifier.checkGroup(null)); - Throwable excep5 = - assertThrows(IllegalNamespaceException.class, () -> NameIdentifier.checkGroup(abc)); - assertTrue(excep5.getMessage().contains("Group namespace must be non-null and have 3 levels")); - - // test role - assertThrows(IllegalNameIdentifierException.class, () -> NameIdentifier.checkRole(null)); - Throwable excep6 = - assertThrows(IllegalNamespaceException.class, () -> NameIdentifier.checkRole(abc)); - assertTrue(excep6.getMessage().contains("Role namespace must be non-null and have 3 levels")); } } diff --git a/api/src/test/java/com/datastrato/gravitino/TestNamespace.java b/api/src/test/java/com/datastrato/gravitino/TestNamespace.java index 113f23fbec7..8b12b75c22f 100644 --- a/api/src/test/java/com/datastrato/gravitino/TestNamespace.java +++ b/api/src/test/java/com/datastrato/gravitino/TestNamespace.java @@ -67,26 +67,5 @@ public void testCheckNamespace() { Assertions.assertThrows(IllegalNamespaceException.class, () -> Namespace.checkTable(abcd)); Assertions.assertTrue( excep3.getMessage().contains("Table namespace must be non-null and have 3 levels")); - - // Test user - Assertions.assertThrows(IllegalNamespaceException.class, () -> Namespace.checkUser(null)); - Throwable excep4 = - Assertions.assertThrows(IllegalNamespaceException.class, () -> Namespace.checkUser(abcd)); - Assertions.assertTrue( - excep4.getMessage().contains("User namespace must be non-null and have 3 levels")); - - // Test group - Assertions.assertThrows(IllegalNamespaceException.class, () -> Namespace.checkGroup(null)); - Throwable excep5 = - Assertions.assertThrows(IllegalNamespaceException.class, () -> Namespace.checkGroup(abcd)); - Assertions.assertTrue( - excep5.getMessage().contains("Group namespace must be non-null and have 3 levels")); - - // Test role - Assertions.assertThrows(IllegalNamespaceException.class, () -> Namespace.checkRole(null)); - Throwable excep6 = - Assertions.assertThrows(IllegalNamespaceException.class, () -> Namespace.checkRole(abcd)); - Assertions.assertTrue( - excep6.getMessage().contains("Role namespace must be non-null and have 3 levels")); } } diff --git a/core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java b/core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java index 05475abc9fa..f3f604d9059 100644 --- a/core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java +++ b/core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java @@ -9,9 +9,12 @@ import com.datastrato.gravitino.GravitinoEnv; import com.datastrato.gravitino.NameIdentifier; import com.datastrato.gravitino.Namespace; +import com.datastrato.gravitino.exceptions.IllegalNameIdentifierException; import com.datastrato.gravitino.exceptions.NoSuchMetalakeException; import com.datastrato.gravitino.meta.CatalogEntity; import com.datastrato.gravitino.meta.SchemaEntity; +import com.google.errorprone.annotations.FormatMethod; +import com.google.errorprone.annotations.FormatString; import java.io.IOException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -69,4 +72,47 @@ public static Namespace ofGroupNamespace(String metalake) { public static Namespace ofUserNamespace(String metalake) { return Namespace.of(metalake, Entity.SYSTEM_CATALOG_RESERVED_NAME, Entity.USER_SCHEMA_NAME); } + + public static void checkUser(NameIdentifier ident) { + check(ident != null, "User identifier must not be null"); + checkUserNamespace(ident.namespace()); + } + + public static void checkGroup(NameIdentifier ident) { + check(ident != null, "Group identifier must not be null"); + checkGroupNamespace(ident.namespace()); + } + + public static void checkRole(NameIdentifier ident) { + check(ident != null, "Role identifier must not be null"); + checkRoleNamespace(ident.namespace()); + } + + public static void checkUserNamespace(Namespace namespace) { + check( + namespace != null && namespace.length() == 3, + "User namespace must be non-null and have 3 levels, the input namespace is %s", + namespace); + } + + public static void checkGroupNamespace(Namespace namespace) { + check( + namespace != null && namespace.length() == 3, + "Group namespace must be non-null and have 3 levels, the input namespace is %s", + namespace); + } + + public static void checkRoleNamespace(Namespace namespace) { + check( + namespace != null && namespace.length() == 3, + "Role namespace must be non-null and have 3 levels, the input namespace is %s", + namespace); + } + + @FormatMethod + private static void check(boolean condition, @FormatString String message, Object... args) { + if (!condition) { + throw new IllegalNameIdentifierException(message, args); + } + } } diff --git a/core/src/main/java/com/datastrato/gravitino/storage/relational/service/GroupMetaService.java b/core/src/main/java/com/datastrato/gravitino/storage/relational/service/GroupMetaService.java index 9d19312568d..966c20655fd 100644 --- a/core/src/main/java/com/datastrato/gravitino/storage/relational/service/GroupMetaService.java +++ b/core/src/main/java/com/datastrato/gravitino/storage/relational/service/GroupMetaService.java @@ -7,6 +7,7 @@ import com.datastrato.gravitino.Entity; import com.datastrato.gravitino.HasIdentifier; import com.datastrato.gravitino.NameIdentifier; +import com.datastrato.gravitino.authorization.AuthorizationUtils; import com.datastrato.gravitino.exceptions.NoSuchEntityException; import com.datastrato.gravitino.meta.GroupEntity; import com.datastrato.gravitino.storage.relational.mapper.GroupMetaMapper; @@ -67,7 +68,7 @@ private Long getGroupIdByMetalakeIdAndName(Long metalakeId, String groupName) { } public GroupEntity getGroupByIdentifier(NameIdentifier identifier) { - NameIdentifier.checkGroup(identifier); + AuthorizationUtils.checkGroup(identifier); Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0)); @@ -79,7 +80,7 @@ public GroupEntity getGroupByIdentifier(NameIdentifier identifier) { public void insertGroup(GroupEntity groupEntity, boolean overwritten) { try { - NameIdentifier.checkGroup(groupEntity.nameIdentifier()); + AuthorizationUtils.checkGroup(groupEntity.nameIdentifier()); Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(groupEntity.namespace().level(0)); @@ -123,7 +124,7 @@ public void insertGroup(GroupEntity groupEntity, boolean overwritten) { } public boolean deleteGroup(NameIdentifier identifier) { - NameIdentifier.checkGroup(identifier); + AuthorizationUtils.checkGroup(identifier); Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0)); @@ -142,7 +143,7 @@ public boolean deleteGroup(NameIdentifier identifier) { public GroupEntity updateGroup( NameIdentifier identifier, Function updater) { - NameIdentifier.checkGroup(identifier); + AuthorizationUtils.checkGroup(identifier); Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0)); diff --git a/core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java b/core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java index b0ad60740d3..88f34f0cd40 100644 --- a/core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java +++ b/core/src/main/java/com/datastrato/gravitino/storage/relational/service/RoleMetaService.java @@ -6,6 +6,7 @@ import com.datastrato.gravitino.Entity; import com.datastrato.gravitino.NameIdentifier; +import com.datastrato.gravitino.authorization.AuthorizationUtils; import com.datastrato.gravitino.exceptions.NoSuchEntityException; import com.datastrato.gravitino.meta.RoleEntity; import com.datastrato.gravitino.storage.relational.mapper.GroupRoleRelMapper; @@ -69,7 +70,7 @@ public List listRolesByGroupId(Long groupId) { public void insertRole(RoleEntity roleEntity, boolean overwritten) { try { - NameIdentifier.checkRole(roleEntity.nameIdentifier()); + AuthorizationUtils.checkRole(roleEntity.nameIdentifier()); Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(roleEntity.namespace().level(0)); @@ -94,7 +95,7 @@ public void insertRole(RoleEntity roleEntity, boolean overwritten) { } public RoleEntity getRoleByIdentifier(NameIdentifier identifier) { - NameIdentifier.checkRole(identifier); + AuthorizationUtils.checkRole(identifier); Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0)); @@ -104,7 +105,7 @@ public RoleEntity getRoleByIdentifier(NameIdentifier identifier) { } public boolean deleteRole(NameIdentifier identifier) { - NameIdentifier.checkRole(identifier); + AuthorizationUtils.checkRole(identifier); Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0)); diff --git a/core/src/main/java/com/datastrato/gravitino/storage/relational/service/UserMetaService.java b/core/src/main/java/com/datastrato/gravitino/storage/relational/service/UserMetaService.java index 378bb807132..13da552d9ef 100644 --- a/core/src/main/java/com/datastrato/gravitino/storage/relational/service/UserMetaService.java +++ b/core/src/main/java/com/datastrato/gravitino/storage/relational/service/UserMetaService.java @@ -7,6 +7,7 @@ import com.datastrato.gravitino.Entity; import com.datastrato.gravitino.HasIdentifier; import com.datastrato.gravitino.NameIdentifier; +import com.datastrato.gravitino.authorization.AuthorizationUtils; import com.datastrato.gravitino.exceptions.NoSuchEntityException; import com.datastrato.gravitino.meta.UserEntity; import com.datastrato.gravitino.storage.relational.mapper.UserMetaMapper; @@ -67,7 +68,7 @@ private Long getUserIdByMetalakeIdAndName(Long metalakeId, String userName) { } public UserEntity getUserByIdentifier(NameIdentifier identifier) { - NameIdentifier.checkUser(identifier); + AuthorizationUtils.checkUser(identifier); Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0)); @@ -79,7 +80,7 @@ public UserEntity getUserByIdentifier(NameIdentifier identifier) { public void insertUser(UserEntity userEntity, boolean overwritten) { try { - NameIdentifier.checkUser(userEntity.nameIdentifier()); + AuthorizationUtils.checkUser(userEntity.nameIdentifier()); Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(userEntity.namespace().level(0)); @@ -123,7 +124,7 @@ public void insertUser(UserEntity userEntity, boolean overwritten) { } public boolean deleteUser(NameIdentifier identifier) { - NameIdentifier.checkUser(identifier); + AuthorizationUtils.checkUser(identifier); Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0)); @@ -141,7 +142,7 @@ public boolean deleteUser(NameIdentifier identifier) { public UserEntity updateUser( NameIdentifier identifier, Function updater) { - NameIdentifier.checkUser(identifier); + AuthorizationUtils.checkUser(identifier); Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(identifier.namespace().level(0)); diff --git a/core/src/test/java/com/datastrato/gravitino/authorization/TestAuthorizationUtils.java b/core/src/test/java/com/datastrato/gravitino/authorization/TestAuthorizationUtils.java new file mode 100644 index 00000000000..c69b5e828ad --- /dev/null +++ b/core/src/test/java/com/datastrato/gravitino/authorization/TestAuthorizationUtils.java @@ -0,0 +1,115 @@ +package com.datastrato.gravitino.authorization; + +import com.datastrato.gravitino.NameIdentifier; +import com.datastrato.gravitino.Namespace; +import com.datastrato.gravitino.exceptions.IllegalNameIdentifierException; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +class TestAuthorizationUtils { + + String metalake = "metalake"; + + @Test + void testCreateNameIdentifier() { + NameIdentifier user = AuthorizationUtils.ofUser(metalake, "user"); + NameIdentifier group = AuthorizationUtils.ofGroup(metalake, "group"); + NameIdentifier role = AuthorizationUtils.ofRole(metalake, "role"); + + Assertions.assertEquals(AuthorizationUtils.ofUserNamespace(metalake), user.namespace()); + Assertions.assertEquals("user", user.name()); + Assertions.assertEquals(AuthorizationUtils.ofGroupNamespace(metalake), group.namespace()); + Assertions.assertEquals("group", group.name()); + Assertions.assertEquals(AuthorizationUtils.ofRoleNamespace(metalake), role.namespace()); + Assertions.assertEquals("role", role.name()); + + Assertions.assertThrows( + IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofUser(metalake, null)); + Assertions.assertThrows( + IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofUser(metalake, "")); + Assertions.assertThrows( + IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofGroup(metalake, null)); + Assertions.assertThrows( + IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofGroup(metalake, "")); + Assertions.assertThrows( + IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofRole(metalake, null)); + Assertions.assertThrows( + IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofRole(metalake, "")); + } + + @Test + void testCreateNamespace() { + Namespace namespace = AuthorizationUtils.ofUserNamespace(metalake); + Assertions.assertEquals(3, namespace.length()); + Assertions.assertEquals(metalake, namespace.level(0)); + Assertions.assertEquals("system", namespace.level(1)); + Assertions.assertEquals("user", namespace.level(2)); + + namespace = AuthorizationUtils.ofGroupNamespace(metalake); + Assertions.assertEquals(3, namespace.length()); + Assertions.assertEquals(metalake, namespace.level(0)); + Assertions.assertEquals("system", namespace.level(1)); + Assertions.assertEquals("group", namespace.level(2)); + + namespace = AuthorizationUtils.ofRoleNamespace(metalake); + Assertions.assertEquals(3, namespace.length()); + Assertions.assertEquals(metalake, namespace.level(0)); + Assertions.assertEquals("system", namespace.level(1)); + Assertions.assertEquals("role", namespace.level(2)); + + Assertions.assertThrows( + IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofUserNamespace(null)); + Assertions.assertThrows( + IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofUserNamespace("")); + Assertions.assertThrows( + IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofGroupNamespace(null)); + Assertions.assertThrows( + IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofGroupNamespace("")); + Assertions.assertThrows( + IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofRoleNamespace(null)); + Assertions.assertThrows( + IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofRoleNamespace("")); + } + + @Test + void testCheckNameIdentifier() { + NameIdentifier user = AuthorizationUtils.ofUser(metalake, "user"); + NameIdentifier group = AuthorizationUtils.ofGroup(metalake, "group"); + NameIdentifier role = AuthorizationUtils.ofRole(metalake, "role"); + + Assertions.assertDoesNotThrow(() -> AuthorizationUtils.checkUser(user)); + Assertions.assertDoesNotThrow(() -> AuthorizationUtils.checkGroup(group)); + Assertions.assertDoesNotThrow(() -> AuthorizationUtils.checkRole(role)); + + Assertions.assertThrows( + IllegalNameIdentifierException.class, + () -> AuthorizationUtils.checkUser(NameIdentifier.of("a", "b"))); + Assertions.assertThrows( + IllegalNameIdentifierException.class, + () -> AuthorizationUtils.checkGroup(NameIdentifier.of("a"))); + Assertions.assertThrows( + IllegalNameIdentifierException.class, + () -> AuthorizationUtils.checkRole(NameIdentifier.of("a", "b", "c"))); + } + + @Test + void testCheckNamespace() { + Namespace userNamespace = AuthorizationUtils.ofUserNamespace(metalake); + Namespace groupNamespace = AuthorizationUtils.ofGroupNamespace(metalake); + Namespace roleNamespace = AuthorizationUtils.ofRoleNamespace(metalake); + + Assertions.assertDoesNotThrow(() -> AuthorizationUtils.checkUserNamespace(userNamespace)); + Assertions.assertDoesNotThrow(() -> AuthorizationUtils.checkGroupNamespace(groupNamespace)); + Assertions.assertDoesNotThrow(() -> AuthorizationUtils.checkRoleNamespace(roleNamespace)); + + Assertions.assertThrows( + IllegalNameIdentifierException.class, + () -> AuthorizationUtils.checkUserNamespace(Namespace.of("a", "b"))); + Assertions.assertThrows( + IllegalNameIdentifierException.class, + () -> AuthorizationUtils.checkGroupNamespace(Namespace.of("a"))); + Assertions.assertThrows( + IllegalNameIdentifierException.class, + () -> AuthorizationUtils.checkRoleNamespace(Namespace.of("a", "b", "c", "d"))); + } +} From be91367336b89c95e8b76dbf917302c1b16cdd47 Mon Sep 17 00:00:00 2001 From: yangliwei Date: Wed, 24 Apr 2024 10:40:09 +0800 Subject: [PATCH 4/6] fix: test --- .../authorization/AuthorizationUtils.java | 22 ++++-------- .../authorization/TestAuthorizationUtils.java | 35 ++++++++++++------- 2 files changed, 29 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java b/core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java index f3f604d9059..a9e3128ad15 100644 --- a/core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java +++ b/core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java @@ -9,12 +9,9 @@ import com.datastrato.gravitino.GravitinoEnv; import com.datastrato.gravitino.NameIdentifier; import com.datastrato.gravitino.Namespace; -import com.datastrato.gravitino.exceptions.IllegalNameIdentifierException; import com.datastrato.gravitino.exceptions.NoSuchMetalakeException; import com.datastrato.gravitino.meta.CatalogEntity; import com.datastrato.gravitino.meta.SchemaEntity; -import com.google.errorprone.annotations.FormatMethod; -import com.google.errorprone.annotations.FormatString; import java.io.IOException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -74,45 +71,38 @@ public static Namespace ofUserNamespace(String metalake) { } public static void checkUser(NameIdentifier ident) { - check(ident != null, "User identifier must not be null"); + NameIdentifier.check(ident != null, "User identifier must not be null"); checkUserNamespace(ident.namespace()); } public static void checkGroup(NameIdentifier ident) { - check(ident != null, "Group identifier must not be null"); + NameIdentifier.check(ident != null, "Group identifier must not be null"); checkGroupNamespace(ident.namespace()); } public static void checkRole(NameIdentifier ident) { - check(ident != null, "Role identifier must not be null"); + NameIdentifier.check(ident != null, "Role identifier must not be null"); checkRoleNamespace(ident.namespace()); } public static void checkUserNamespace(Namespace namespace) { - check( + Namespace.check( namespace != null && namespace.length() == 3, "User namespace must be non-null and have 3 levels, the input namespace is %s", namespace); } public static void checkGroupNamespace(Namespace namespace) { - check( + Namespace.check( namespace != null && namespace.length() == 3, "Group namespace must be non-null and have 3 levels, the input namespace is %s", namespace); } public static void checkRoleNamespace(Namespace namespace) { - check( + Namespace.check( namespace != null && namespace.length() == 3, "Role namespace must be non-null and have 3 levels, the input namespace is %s", namespace); } - - @FormatMethod - private static void check(boolean condition, @FormatString String message, Object... args) { - if (!condition) { - throw new IllegalNameIdentifierException(message, args); - } - } } diff --git a/core/src/test/java/com/datastrato/gravitino/authorization/TestAuthorizationUtils.java b/core/src/test/java/com/datastrato/gravitino/authorization/TestAuthorizationUtils.java index c69b5e828ad..b7699367a5a 100644 --- a/core/src/test/java/com/datastrato/gravitino/authorization/TestAuthorizationUtils.java +++ b/core/src/test/java/com/datastrato/gravitino/authorization/TestAuthorizationUtils.java @@ -1,8 +1,13 @@ +/* + * Copyright 2024 Datastrato Pvt Ltd. + * This software is licensed under the Apache License version 2. + */ package com.datastrato.gravitino.authorization; import com.datastrato.gravitino.NameIdentifier; import com.datastrato.gravitino.Namespace; import com.datastrato.gravitino.exceptions.IllegalNameIdentifierException; +import com.datastrato.gravitino.exceptions.IllegalNamespaceException; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -22,7 +27,10 @@ void testCreateNameIdentifier() { Assertions.assertEquals("group", group.name()); Assertions.assertEquals(AuthorizationUtils.ofRoleNamespace(metalake), role.namespace()); Assertions.assertEquals("role", role.name()); + } + @Test + void testCreateNameIdentifierWithInvalidArgs() { Assertions.assertThrows( IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofUser(metalake, null)); Assertions.assertThrows( @@ -56,19 +64,22 @@ void testCreateNamespace() { Assertions.assertEquals(metalake, namespace.level(0)); Assertions.assertEquals("system", namespace.level(1)); Assertions.assertEquals("role", namespace.level(2)); + } + @Test + void testCreateNamespaceWithInvalidArgs() { Assertions.assertThrows( - IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofUserNamespace(null)); + IllegalNamespaceException.class, () -> AuthorizationUtils.ofUserNamespace(null)); Assertions.assertThrows( - IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofUserNamespace("")); + IllegalNamespaceException.class, () -> AuthorizationUtils.ofUserNamespace("")); Assertions.assertThrows( - IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofGroupNamespace(null)); + IllegalNamespaceException.class, () -> AuthorizationUtils.ofGroupNamespace(null)); Assertions.assertThrows( - IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofGroupNamespace("")); + IllegalNamespaceException.class, () -> AuthorizationUtils.ofGroupNamespace("")); Assertions.assertThrows( - IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofRoleNamespace(null)); + IllegalNamespaceException.class, () -> AuthorizationUtils.ofRoleNamespace(null)); Assertions.assertThrows( - IllegalNameIdentifierException.class, () -> AuthorizationUtils.ofRoleNamespace("")); + IllegalNamespaceException.class, () -> AuthorizationUtils.ofRoleNamespace("")); } @Test @@ -83,13 +94,13 @@ void testCheckNameIdentifier() { Assertions.assertThrows( IllegalNameIdentifierException.class, - () -> AuthorizationUtils.checkUser(NameIdentifier.of("a", "b"))); + () -> AuthorizationUtils.checkUser(NameIdentifier.of(""))); Assertions.assertThrows( IllegalNameIdentifierException.class, - () -> AuthorizationUtils.checkGroup(NameIdentifier.of("a"))); + () -> AuthorizationUtils.checkGroup(NameIdentifier.of(""))); Assertions.assertThrows( IllegalNameIdentifierException.class, - () -> AuthorizationUtils.checkRole(NameIdentifier.of("a", "b", "c"))); + () -> AuthorizationUtils.checkRole(NameIdentifier.of(""))); } @Test @@ -103,13 +114,13 @@ void testCheckNamespace() { Assertions.assertDoesNotThrow(() -> AuthorizationUtils.checkRoleNamespace(roleNamespace)); Assertions.assertThrows( - IllegalNameIdentifierException.class, + IllegalNamespaceException.class, () -> AuthorizationUtils.checkUserNamespace(Namespace.of("a", "b"))); Assertions.assertThrows( - IllegalNameIdentifierException.class, + IllegalNamespaceException.class, () -> AuthorizationUtils.checkGroupNamespace(Namespace.of("a"))); Assertions.assertThrows( - IllegalNameIdentifierException.class, + IllegalNamespaceException.class, () -> AuthorizationUtils.checkRoleNamespace(Namespace.of("a", "b", "c", "d"))); } } From 48efb42f12fd08b596740bf25f32df54873deb69 Mon Sep 17 00:00:00 2001 From: yangliwei Date: Wed, 24 Apr 2024 11:08:51 +0800 Subject: [PATCH 5/6] fix: test --- .../authorization/TestAuthorizationUtils.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/core/src/test/java/com/datastrato/gravitino/authorization/TestAuthorizationUtils.java b/core/src/test/java/com/datastrato/gravitino/authorization/TestAuthorizationUtils.java index b7699367a5a..67211d8b1e5 100644 --- a/core/src/test/java/com/datastrato/gravitino/authorization/TestAuthorizationUtils.java +++ b/core/src/test/java/com/datastrato/gravitino/authorization/TestAuthorizationUtils.java @@ -92,6 +92,12 @@ void testCheckNameIdentifier() { Assertions.assertDoesNotThrow(() -> AuthorizationUtils.checkGroup(group)); Assertions.assertDoesNotThrow(() -> AuthorizationUtils.checkRole(role)); + Assertions.assertThrows( + IllegalNameIdentifierException.class, () -> AuthorizationUtils.checkUser(null)); + Assertions.assertThrows( + IllegalNameIdentifierException.class, () -> AuthorizationUtils.checkGroup(null)); + Assertions.assertThrows( + IllegalNameIdentifierException.class, () -> AuthorizationUtils.checkRole(null)); Assertions.assertThrows( IllegalNameIdentifierException.class, () -> AuthorizationUtils.checkUser(NameIdentifier.of(""))); @@ -113,6 +119,12 @@ void testCheckNamespace() { Assertions.assertDoesNotThrow(() -> AuthorizationUtils.checkGroupNamespace(groupNamespace)); Assertions.assertDoesNotThrow(() -> AuthorizationUtils.checkRoleNamespace(roleNamespace)); + Assertions.assertThrows( + IllegalNamespaceException.class, () -> AuthorizationUtils.checkUserNamespace(null)); + Assertions.assertThrows( + IllegalNamespaceException.class, () -> AuthorizationUtils.checkGroupNamespace(null)); + Assertions.assertThrows( + IllegalNamespaceException.class, () -> AuthorizationUtils.checkRoleNamespace(null)); Assertions.assertThrows( IllegalNamespaceException.class, () -> AuthorizationUtils.checkUserNamespace(Namespace.of("a", "b"))); From cdc3e4fd537d5bc37d1d38c0ab400d57010f2688 Mon Sep 17 00:00:00 2001 From: yangliwei Date: Wed, 24 Apr 2024 11:38:11 +0800 Subject: [PATCH 6/6] fix: msg --- .../gravitino/authorization/AuthorizationUtils.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java b/core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java index a9e3128ad15..821fdd075f6 100644 --- a/core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java +++ b/core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java @@ -88,21 +88,21 @@ public static void checkRole(NameIdentifier ident) { public static void checkUserNamespace(Namespace namespace) { Namespace.check( namespace != null && namespace.length() == 3, - "User namespace must be non-null and have 3 levels, the input namespace is %s", + "User namespace must have 3 levels, the input namespace is %s", namespace); } public static void checkGroupNamespace(Namespace namespace) { Namespace.check( namespace != null && namespace.length() == 3, - "Group namespace must be non-null and have 3 levels, the input namespace is %s", + "Group namespace must have 3 levels, the input namespace is %s", namespace); } public static void checkRoleNamespace(Namespace namespace) { Namespace.check( namespace != null && namespace.length() == 3, - "Role namespace must be non-null and have 3 levels, the input namespace is %s", + "Role namespace must have 3 levels, the input namespace is %s", namespace); } }