From c3172952c7d70671641eb36a020c3cd12a84f0a8 Mon Sep 17 00:00:00 2001 From: roryqi Date: Fri, 3 Jan 2025 17:36:57 +0800 Subject: [PATCH 1/2] [#6060] fix(core): Add the check of requests related to authorization (#6065) ### What changes were proposed in this pull request? Add the check of requests related to authorization ### Why are the changes needed? Fix: #6060 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added UT. --- .../dto/requests/RoleCreateRequest.java | 9 +++ .../server/web/rest/GroupOperations.java | 12 ++-- .../server/web/rest/OwnerOperations.java | 1 + .../server/web/rest/PermissionOperations.java | 60 +++++++++++-------- .../server/web/rest/RoleOperations.java | 1 + .../server/web/rest/UserOperations.java | 12 ++-- .../server/web/rest/TestGroupOperations.java | 9 +++ .../server/web/rest/TestOwnerOperations.java | 8 +++ .../web/rest/TestPermissionOperations.java | 47 ++++++++++++++- .../server/web/rest/TestRoleOperations.java | 31 +++++++++- .../server/web/rest/TestUserOperations.java | 9 +++ 11 files changed, 163 insertions(+), 36 deletions(-) diff --git a/common/src/main/java/org/apache/gravitino/dto/requests/RoleCreateRequest.java b/common/src/main/java/org/apache/gravitino/dto/requests/RoleCreateRequest.java index 9d85c0c6e14..0466d7d3105 100644 --- a/common/src/main/java/org/apache/gravitino/dto/requests/RoleCreateRequest.java +++ b/common/src/main/java/org/apache/gravitino/dto/requests/RoleCreateRequest.java @@ -79,5 +79,14 @@ public void validate() throws IllegalArgumentException { Preconditions.checkArgument( StringUtils.isNotBlank(name), "\"name\" field is required and cannot be empty"); Preconditions.checkArgument(securableObjects != null, "\"securableObjects\" can't null "); + for (SecurableObjectDTO objectDTO : securableObjects) { + Preconditions.checkArgument( + StringUtils.isNotBlank(objectDTO.name()), "\" securable object name\" can't be blank"); + Preconditions.checkArgument( + objectDTO.type() != null, "\" securable object type\" can't be null"); + Preconditions.checkArgument( + objectDTO.privileges() != null && !objectDTO.privileges().isEmpty(), + "\"securable object privileges\" can't be null or empty"); + } } } diff --git a/server/src/main/java/org/apache/gravitino/server/web/rest/GroupOperations.java b/server/src/main/java/org/apache/gravitino/server/web/rest/GroupOperations.java index 12cf769932e..95db0ca67f3 100644 --- a/server/src/main/java/org/apache/gravitino/server/web/rest/GroupOperations.java +++ b/server/src/main/java/org/apache/gravitino/server/web/rest/GroupOperations.java @@ -103,11 +103,13 @@ public Response addGroup(@PathParam("metalake") String metalake, GroupAddRequest TreeLockUtils.doWithTreeLock( NameIdentifier.of(AuthorizationUtils.ofGroupNamespace(metalake).levels()), LockType.WRITE, - () -> - Utils.ok( - new GroupResponse( - DTOConverters.toDTO( - accessControlManager.addGroup(metalake, request.getName())))))); + () -> { + request.validate(); + return Utils.ok( + new GroupResponse( + DTOConverters.toDTO( + accessControlManager.addGroup(metalake, request.getName())))); + })); } catch (Exception e) { return ExceptionHandlers.handleGroupException( OperationType.ADD, request.getName(), metalake, e); 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 ea5684b55f9..7dcfcfd0674 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 @@ -113,6 +113,7 @@ public Response setOwnerForObject( return Utils.doAs( httpRequest, () -> { + request.validate(); MetadataObjectUtil.checkMetadataObject(metalake, object); NameIdentifier objectIdent = MetadataObjectUtil.toEntityIdent(metalake, object); TreeLockUtils.doWithTreeLock( 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 38fcd7380e6..3ce1517a46a 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 @@ -87,12 +87,14 @@ public Response grantRolesToUser( TreeLockUtils.doWithTreeLock( NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()), LockType.READ, - () -> - Utils.ok( - new UserResponse( - DTOConverters.toDTO( - accessControlManager.grantRolesToUser( - metalake, request.getRoleNames(), user))))))); + () -> { + request.validate(); + return Utils.ok( + new UserResponse( + DTOConverters.toDTO( + accessControlManager.grantRolesToUser( + metalake, request.getRoleNames(), user)))); + }))); } catch (Exception e) { return ExceptionHandlers.handleUserPermissionOperationException( OperationType.GRANT, StringUtils.join(request.getRoleNames(), ","), user, e); @@ -119,12 +121,14 @@ public Response grantRolesToGroup( TreeLockUtils.doWithTreeLock( NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()), LockType.READ, - () -> - Utils.ok( - new GroupResponse( - DTOConverters.toDTO( - accessControlManager.grantRolesToGroup( - metalake, request.getRoleNames(), group))))))); + () -> { + request.validate(); + return Utils.ok( + new GroupResponse( + DTOConverters.toDTO( + accessControlManager.grantRolesToGroup( + metalake, request.getRoleNames(), group)))); + }))); } catch (Exception e) { return ExceptionHandlers.handleGroupPermissionOperationException( OperationType.GRANT, StringUtils.join(request.getRoleNames(), ","), group, e); @@ -151,12 +155,14 @@ public Response revokeRolesFromUser( TreeLockUtils.doWithTreeLock( NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()), LockType.READ, - () -> - Utils.ok( - new UserResponse( - DTOConverters.toDTO( - accessControlManager.revokeRolesFromUser( - metalake, request.getRoleNames(), user))))))); + () -> { + request.validate(); + return Utils.ok( + new UserResponse( + DTOConverters.toDTO( + accessControlManager.revokeRolesFromUser( + metalake, request.getRoleNames(), user)))); + }))); } catch (Exception e) { return ExceptionHandlers.handleUserPermissionOperationException( OperationType.REVOKE, StringUtils.join(request.getRoleNames(), ","), user, e); @@ -183,12 +189,14 @@ public Response revokeRolesFromGroup( TreeLockUtils.doWithTreeLock( NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()), LockType.READ, - () -> - Utils.ok( - new GroupResponse( - DTOConverters.toDTO( - accessControlManager.revokeRolesFromGroup( - metalake, request.getRoleNames(), group))))))); + () -> { + request.validate(); + return Utils.ok( + new GroupResponse( + DTOConverters.toDTO( + accessControlManager.revokeRolesFromGroup( + metalake, request.getRoleNames(), group)))); + }))); } catch (Exception e) { return ExceptionHandlers.handleGroupPermissionOperationException( OperationType.REVOKE, StringUtils.join(request.getRoleNames()), group, e); @@ -214,6 +222,8 @@ public Response grantPrivilegeToRole( return Utils.doAs( httpRequest, () -> { + privilegeGrantRequest.validate(); + for (PrivilegeDTO privilegeDTO : privilegeGrantRequest.getPrivileges()) { AuthorizationUtils.checkPrivilege(privilegeDTO, object, metalake); } @@ -259,6 +269,8 @@ public Response revokePrivilegeFromRole( return Utils.doAs( httpRequest, () -> { + privilegeRevokeRequest.validate(); + for (PrivilegeDTO privilegeDTO : privilegeRevokeRequest.getPrivileges()) { AuthorizationUtils.checkPrivilege(privilegeDTO, object, metalake); } 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 bf82d78b676..9690afe13f1 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 @@ -127,6 +127,7 @@ public Response createRole(@PathParam("metalake") String metalake, RoleCreateReq return Utils.doAs( httpRequest, () -> { + request.validate(); Set metadataObjects = Sets.newHashSet(); for (SecurableObjectDTO object : request.getSecurableObjects()) { MetadataObject metadataObject = diff --git a/server/src/main/java/org/apache/gravitino/server/web/rest/UserOperations.java b/server/src/main/java/org/apache/gravitino/server/web/rest/UserOperations.java index 24f34d652ab..518178cd325 100644 --- a/server/src/main/java/org/apache/gravitino/server/web/rest/UserOperations.java +++ b/server/src/main/java/org/apache/gravitino/server/web/rest/UserOperations.java @@ -129,11 +129,13 @@ public Response addUser(@PathParam("metalake") String metalake, UserAddRequest r TreeLockUtils.doWithTreeLock( NameIdentifier.of(AuthorizationUtils.ofGroupNamespace(metalake).levels()), LockType.WRITE, - () -> - Utils.ok( - new UserResponse( - DTOConverters.toDTO( - accessControlManager.addUser(metalake, request.getName())))))); + () -> { + request.validate(); + return Utils.ok( + new UserResponse( + DTOConverters.toDTO( + accessControlManager.addUser(metalake, request.getName())))); + })); } catch (Exception e) { return ExceptionHandlers.handleUserException( OperationType.ADD, request.getName(), metalake, e); diff --git a/server/src/test/java/org/apache/gravitino/server/web/rest/TestGroupOperations.java b/server/src/test/java/org/apache/gravitino/server/web/rest/TestGroupOperations.java index 77f0cf97988..ac4f8c66a8d 100644 --- a/server/src/test/java/org/apache/gravitino/server/web/rest/TestGroupOperations.java +++ b/server/src/test/java/org/apache/gravitino/server/web/rest/TestGroupOperations.java @@ -116,6 +116,15 @@ public void testAddGroup() { when(manager.addGroup(any(), any())).thenReturn(group); + // test with IllegalRequest + GroupAddRequest illegalReq = new GroupAddRequest(""); + Response illegalResp = + target("/metalakes/metalake1/groups") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .post(Entity.entity(illegalReq, MediaType.APPLICATION_JSON_TYPE)); + Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), illegalResp.getStatus()); + Response resp = target("/metalakes/metalake1/groups") .request(MediaType.APPLICATION_JSON_TYPE) 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 0643ed9bf1a..dc7451a538c 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 @@ -202,6 +202,14 @@ public Type type() { @Test void testSetOwnerForObject() { when(metalakeDispatcher.metalakeExists(any())).thenReturn(true); + OwnerSetRequest invalidRequest = new OwnerSetRequest(null, Owner.Type.USER); + Response invalidResp = + target("/metalakes/metalake1/owners/metalake/metalake1") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .put(Entity.entity(invalidRequest, MediaType.APPLICATION_JSON_TYPE)); + Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), invalidResp.getStatus()); + OwnerSetRequest request = new OwnerSetRequest("test", Owner.Type.USER); Response resp = target("/metalakes/metalake1/owners/metalake/metalake1") diff --git a/server/src/test/java/org/apache/gravitino/server/web/rest/TestPermissionOperations.java b/server/src/test/java/org/apache/gravitino/server/web/rest/TestPermissionOperations.java index 8876e9035f4..1f507cbbcc1 100644 --- a/server/src/test/java/org/apache/gravitino/server/web/rest/TestPermissionOperations.java +++ b/server/src/test/java/org/apache/gravitino/server/web/rest/TestPermissionOperations.java @@ -135,8 +135,15 @@ public void testGrantRolesToUser() { .build(); when(manager.grantRolesToUser(any(), any(), any())).thenReturn(userEntity); - RoleGrantRequest request = new RoleGrantRequest(Lists.newArrayList("role1")); + RoleGrantRequest illegalReq = new RoleGrantRequest(null); + Response illegalResp = + target("/metalakes/metalake1/permissions/users/user/grant") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .put(Entity.entity(illegalReq, MediaType.APPLICATION_JSON_TYPE)); + Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), illegalResp.getStatus()); + RoleGrantRequest request = new RoleGrantRequest(Lists.newArrayList("role1")); Response resp = target("/metalakes/metalake1/permissions/users/user/grant") .request(MediaType.APPLICATION_JSON_TYPE) @@ -232,6 +239,15 @@ public void testGrantRolesToGroup() { .build(); when(manager.grantRolesToGroup(any(), any(), any())).thenReturn(groupEntity); + // Test with Illegal request + RoleGrantRequest illegalReq = new RoleGrantRequest(null); + Response illegalResp = + target("/metalakes/metalake1/permissions/groups/group/grant") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .put(Entity.entity(illegalReq, MediaType.APPLICATION_JSON_TYPE)); + Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), illegalResp.getStatus()); + RoleGrantRequest request = new RoleGrantRequest(Lists.newArrayList("role1")); Response resp = @@ -331,6 +347,16 @@ public void testRevokeRolesFromUser() { AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build()) .build(); when(manager.revokeRolesFromUser(any(), any(), any())).thenReturn(userEntity); + + // Test with illegal request + RoleRevokeRequest illegalReq = new RoleRevokeRequest(null); + Response illegalResp = + target("/metalakes/metalake1/permissions/users/user1/revoke") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .put(Entity.entity(illegalReq, MediaType.APPLICATION_JSON_TYPE)); + Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), illegalResp.getStatus()); + RoleRevokeRequest request = new RoleRevokeRequest(Lists.newArrayList("role1")); Response resp = @@ -393,6 +419,15 @@ public void testRevokeRolesFromGroup() { AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build()) .build(); when(manager.revokeRolesFromGroup(any(), any(), any())).thenReturn(groupEntity); + // Test with illegal request + RoleRevokeRequest illegalReq = new RoleRevokeRequest(null); + Response illegalResp = + target("/metalakes/metalake1/permissions/groups/group1/revoke") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .put(Entity.entity(illegalReq, MediaType.APPLICATION_JSON_TYPE)); + Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), illegalResp.getStatus()); + RoleRevokeRequest request = new RoleRevokeRequest(Lists.newArrayList("role1")); Response resp = @@ -538,6 +573,16 @@ public void testRevokePrivilegesFromRole() { .build(); when(manager.revokePrivilegesFromRole(any(), any(), any(), any())).thenReturn(roleEntity); when(metalakeDispatcher.metalakeExists(any())).thenReturn(true); + + // Test with illegal request + PrivilegeRevokeRequest illegalReq = new PrivilegeRevokeRequest(null); + Response illegalResp = + target("/metalakes/metalake1/permissions/roles/role1/metalake/metalake1/revoke") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .put(Entity.entity(illegalReq, MediaType.APPLICATION_JSON_TYPE)); + Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), illegalResp.getStatus()); + PrivilegeRevokeRequest request = new PrivilegeRevokeRequest( Lists.newArrayList( 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 6edd3339398..06d9fcc27e9 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 @@ -29,6 +29,7 @@ import com.google.common.collect.Lists; import java.io.IOException; +import java.lang.reflect.Field; import java.time.Instant; import java.util.Collections; import java.util.concurrent.atomic.AtomicReference; @@ -52,6 +53,7 @@ import org.apache.gravitino.catalog.SchemaDispatcher; import org.apache.gravitino.catalog.TableDispatcher; import org.apache.gravitino.catalog.TopicDispatcher; +import org.apache.gravitino.dto.authorization.PrivilegeDTO; import org.apache.gravitino.dto.authorization.RoleDTO; import org.apache.gravitino.dto.authorization.SecurableObjectDTO; import org.apache.gravitino.dto.requests.RoleCreateRequest; @@ -142,7 +144,7 @@ protected void configure() { } @Test - public void testCreateRole() { + public void testCreateRole() throws IllegalAccessException, NoSuchFieldException { SecurableObject securableObject = SecurableObjects.ofCatalog("catalog", Lists.newArrayList(Privileges.UseCatalog.allow())); SecurableObject anotherSecurableObject = @@ -161,6 +163,33 @@ public void testCreateRole() { when(manager.createRole(any(), any(), any(), any())).thenReturn(role); when(catalogDispatcher.catalogExists(any())).thenReturn(true); + // Test with IllegalRequest + RoleCreateRequest illegalRequest = new RoleCreateRequest("role", Collections.emptyMap(), null); + Response illegalResp = + target("/metalakes/metalake1/roles") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .post(Entity.entity(illegalRequest, MediaType.APPLICATION_JSON_TYPE)); + Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), illegalResp.getStatus()); + + SecurableObjectDTO illegalObject = + DTOConverters.toDTO( + SecurableObjects.ofCatalog( + "illegal_catalog", Lists.newArrayList(Privileges.CreateSchema.deny()))); + Field field = illegalObject.getClass().getDeclaredField("privileges"); + field.setAccessible(true); + field.set(illegalObject, new PrivilegeDTO[] {}); + + illegalRequest = + new RoleCreateRequest( + "role", Collections.emptyMap(), new SecurableObjectDTO[] {illegalObject}); + illegalResp = + target("/metalakes/metalake1/roles") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .post(Entity.entity(illegalRequest, MediaType.APPLICATION_JSON_TYPE)); + Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), illegalResp.getStatus()); + Response resp = target("/metalakes/metalake1/roles") .request(MediaType.APPLICATION_JSON_TYPE) diff --git a/server/src/test/java/org/apache/gravitino/server/web/rest/TestUserOperations.java b/server/src/test/java/org/apache/gravitino/server/web/rest/TestUserOperations.java index 7f570e779f4..82bc59155ba 100644 --- a/server/src/test/java/org/apache/gravitino/server/web/rest/TestUserOperations.java +++ b/server/src/test/java/org/apache/gravitino/server/web/rest/TestUserOperations.java @@ -115,6 +115,15 @@ public void testAddUser() { when(manager.addUser(any(), any())).thenReturn(user); + // test with IllegalRequest + UserAddRequest illegalReq = new UserAddRequest(""); + Response illegalResp = + target("/metalakes/metalake1/users") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .post(Entity.entity(illegalReq, MediaType.APPLICATION_JSON_TYPE)); + Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), illegalResp.getStatus()); + Response resp = target("/metalakes/metalake1/users") .request(MediaType.APPLICATION_JSON_TYPE) From 2732792e07e73b539c5905df95f18f0bf6a858f3 Mon Sep 17 00:00:00 2001 From: roryqi Date: Fri, 3 Jan 2025 18:23:41 +0800 Subject: [PATCH 2/2] [#6061] fix(core): Fix the issues of list user or group details (#6067) ### What changes were proposed in this pull request? Fix the issues of list user or group details ### Why are the changes needed? Fix: #6061 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added UT. --- .../test/authorization/AccessControlIT.java | 8 ++++++ .../base/GroupMetaBaseSQLProvider.java | 15 ++++++---- .../base/UserMetaBaseSQLProvider.java | 15 ++++++---- .../provider/h2/GroupMetaH2Provider.java | 15 ++++++---- .../provider/h2/UserMetaH2Provider.java | 15 ++++++---- .../GroupMetaPostgreSQLProvider.java | 15 ++++++---- .../UserMetaPostgreSQLProvider.java | 15 ++++++---- .../service/TestGroupMetaService.java | 28 +++++++++++++++++++ .../service/TestUserMetaService.java | 28 +++++++++++++++++++ 9 files changed, 118 insertions(+), 36 deletions(-) diff --git a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java index 78c29433439..268ed20f3ce 100644 --- a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java +++ b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java @@ -121,6 +121,10 @@ void testManageUsers() { users.stream().map(User::name).collect(Collectors.toList())); Assertions.assertEquals(Lists.newArrayList("role1"), users.get(2).roles()); + // ISSUE-6061: Test listUsers with revoked users + metalake.revokeRolesFromUser(Lists.newArrayList("role1"), username); + Assertions.assertEquals(3, metalake.listUsers().length); + // Get a not-existed user Assertions.assertThrows(NoSuchUserException.class, () -> metalake.getUser("not-existed")); @@ -176,6 +180,10 @@ void testManageGroups() { groups.stream().map(Group::name).collect(Collectors.toList())); Assertions.assertEquals(Lists.newArrayList("role2"), groups.get(0).roles()); + // ISSUE-6061: Test listGroups with revoked groups + metalake.revokeRolesFromGroup(Lists.newArrayList("role2"), groupName); + Assertions.assertEquals(2, metalake.listGroups().length); + Assertions.assertTrue(metalake.removeGroup(groupName)); Assertions.assertFalse(metalake.removeGroup(groupName)); diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/GroupMetaBaseSQLProvider.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/GroupMetaBaseSQLProvider.java index a52e1b86144..1f28b771c2d 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/GroupMetaBaseSQLProvider.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/GroupMetaBaseSQLProvider.java @@ -57,16 +57,19 @@ public String listExtendedGroupPOsByMetalakeId(Long metalakeId) { + " JSON_ARRAYAGG(rot.role_id) as roleIds" + " FROM " + GROUP_TABLE_NAME - + " gt LEFT OUTER JOIN " + + " gt LEFT OUTER JOIN (" + + " SELECT * FROM " + GROUP_ROLE_RELATION_TABLE_NAME - + " rt ON rt.group_id = gt.group_id" - + " LEFT OUTER JOIN " + + " WHERE deleted_at = 0)" + + " AS rt ON rt.group_id = gt.group_id" + + " LEFT OUTER JOIN (" + + " SELECT * FROM " + ROLE_TABLE_NAME - + " rot ON rot.role_id = rt.role_id" + + " WHERE deleted_at = 0)" + + " AS rot ON rot.role_id = rt.role_id" + " WHERE " + " gt.deleted_at = 0 AND" - + " (rot.deleted_at = 0 OR rot.deleted_at is NULL) AND" - + " (rt.deleted_at = 0 OR rt.deleted_at is NULL) AND gt.metalake_id = #{metalakeId}" + + " gt.metalake_id = #{metalakeId}" + " GROUP BY gt.group_id"; } diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/UserMetaBaseSQLProvider.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/UserMetaBaseSQLProvider.java index 2a211c24f5e..4e81ae35df9 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/UserMetaBaseSQLProvider.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/UserMetaBaseSQLProvider.java @@ -165,16 +165,19 @@ public String listExtendedUserPOsByMetalakeId(@Param("metalakeId") Long metalake + " JSON_ARRAYAGG(rot.role_id) as roleIds" + " FROM " + USER_TABLE_NAME - + " ut LEFT OUTER JOIN " + + " ut LEFT OUTER JOIN (" + + " SELECT * FROM " + USER_ROLE_RELATION_TABLE_NAME - + " rt ON rt.user_id = ut.user_id" - + " LEFT OUTER JOIN " + + " WHERE deleted_at = 0)" + + " AS rt ON rt.user_id = ut.user_id" + + " LEFT OUTER JOIN (" + + " SELECT * FROM " + ROLE_TABLE_NAME - + " rot ON rot.role_id = rt.role_id" + + " WHERE deleted_at = 0)" + + " AS rot ON rot.role_id = rt.role_id" + " WHERE " + " ut.deleted_at = 0 AND" - + " (rot.deleted_at = 0 OR rot.deleted_at is NULL) AND" - + " (rt.deleted_at = 0 OR rt.deleted_at is NULL) AND ut.metalake_id = #{metalakeId}" + + " ut.metalake_id = #{metalakeId}" + " GROUP BY ut.user_id"; } diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/h2/GroupMetaH2Provider.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/h2/GroupMetaH2Provider.java index 175d9d8ae9a..e975131e090 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/h2/GroupMetaH2Provider.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/h2/GroupMetaH2Provider.java @@ -37,16 +37,19 @@ public String listExtendedGroupPOsByMetalakeId(@Param("metalakeId") Long metalak + " '[' || GROUP_CONCAT('\"' || rot.role_id || '\"') || ']' as roleIds" + " FROM " + GROUP_TABLE_NAME - + " gt LEFT OUTER JOIN " + + " gt LEFT OUTER JOIN (" + + " SELECT * FROM " + GROUP_ROLE_RELATION_TABLE_NAME - + " rt ON rt.group_id = gt.group_id" - + " LEFT OUTER JOIN " + + " WHERE deleted_at = 0)" + + " AS rt ON rt.group_id = gt.group_id" + + " LEFT OUTER JOIN (" + + " SELECT * FROM " + ROLE_TABLE_NAME - + " rot ON rot.role_id = rt.role_id" + + " WHERE deleted_at = 0)" + + " AS rot ON rot.role_id = rt.role_id" + " WHERE " + " gt.deleted_at = 0 AND" - + " (rot.deleted_at = 0 OR rot.deleted_at is NULL) AND" - + " (rt.deleted_at = 0 OR rt.deleted_at is NULL) AND gt.metalake_id = #{metalakeId}" + + " gt.metalake_id = #{metalakeId}" + " GROUP BY gt.group_id"; } } diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/h2/UserMetaH2Provider.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/h2/UserMetaH2Provider.java index be17138ce49..b4fb1614904 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/h2/UserMetaH2Provider.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/h2/UserMetaH2Provider.java @@ -37,16 +37,19 @@ public String listExtendedUserPOsByMetalakeId(@Param("metalakeId") Long metalake + " '[' || GROUP_CONCAT('\"' || rot.role_id || '\"') || ']' as roleIds" + " FROM " + USER_TABLE_NAME - + " ut LEFT OUTER JOIN " + + " ut LEFT OUTER JOIN (" + + " SELECT * FROM " + USER_ROLE_RELATION_TABLE_NAME - + " rt ON rt.user_id = ut.user_id" - + " LEFT OUTER JOIN " + + " WHERE deleted_at = 0)" + + " AS rt ON rt.user_id = ut.user_id" + + " LEFT OUTER JOIN (" + + " SELECT * FROM " + ROLE_TABLE_NAME - + " rot ON rot.role_id = rt.role_id" + + " WHERE deleted_at = 0)" + + " AS rot ON rot.role_id = rt.role_id" + " WHERE " + " ut.deleted_at = 0 AND " - + "(rot.deleted_at = 0 OR rot.deleted_at is NULL) AND " - + "(rt.deleted_at = 0 OR rt.deleted_at is NULL) AND ut.metalake_id = #{metalakeId}" + + " ut.metalake_id = #{metalakeId}" + " GROUP BY ut.user_id"; } } diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/GroupMetaPostgreSQLProvider.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/GroupMetaPostgreSQLProvider.java index 51cf47bf7d7..3ace33f6f84 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/GroupMetaPostgreSQLProvider.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/GroupMetaPostgreSQLProvider.java @@ -80,16 +80,19 @@ public String listExtendedGroupPOsByMetalakeId(Long metalakeId) { + " JSON_AGG(rot.role_id) as roleIds" + " FROM " + GROUP_TABLE_NAME - + " gt LEFT OUTER JOIN " + + " gt LEFT OUTER JOIN (" + + " SELECT * FROM " + GROUP_ROLE_RELATION_TABLE_NAME - + " rt ON rt.group_id = gt.group_id" - + " LEFT OUTER JOIN " + + " WHERE deleted_at = 0)" + + " AS rt ON rt.group_id = gt.group_id" + + " LEFT OUTER JOIN (" + + " SELECT * FROM " + ROLE_TABLE_NAME - + " rot ON rot.role_id = rt.role_id" + + " WHERE deleted_at = 0)" + + " AS rot ON rot.role_id = rt.role_id" + " WHERE " + " gt.deleted_at = 0 AND" - + " (rot.deleted_at = 0 OR rot.deleted_at is NULL) AND" - + " (rt.deleted_at = 0 OR rt.deleted_at is NULL) AND gt.metalake_id = #{metalakeId}" + + " gt.metalake_id = #{metalakeId}" + " GROUP BY gt.group_id"; } } diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/UserMetaPostgreSQLProvider.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/UserMetaPostgreSQLProvider.java index b6ac62b2b87..84ab965582c 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/UserMetaPostgreSQLProvider.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/UserMetaPostgreSQLProvider.java @@ -80,16 +80,19 @@ public String listExtendedUserPOsByMetalakeId(Long metalakeId) { + " JSON_AGG(rot.role_id) as roleIds" + " FROM " + USER_TABLE_NAME - + " ut LEFT OUTER JOIN " + + " ut LEFT OUTER JOIN (" + + " SELECT * FROM " + USER_ROLE_RELATION_TABLE_NAME - + " rt ON rt.user_id = ut.user_id" - + " LEFT OUTER JOIN " + + " WHERE deleted_at = 0)" + + " AS rt ON rt.user_id = ut.user_id" + + " LEFT OUTER JOIN (" + + " SELECT * FROM " + ROLE_TABLE_NAME - + " rot ON rot.role_id = rt.role_id" + + " WHERE deleted_at = 0)" + + " AS rot ON rot.role_id = rt.role_id" + " WHERE " + " ut.deleted_at = 0 AND" - + " (rot.deleted_at = 0 OR rot.deleted_at is NULL) AND" - + " (rt.deleted_at = 0 OR rt.deleted_at is NULL) AND ut.metalake_id = #{metalakeId}" + + " ut.metalake_id = #{metalakeId}" + " GROUP BY ut.user_id"; } } diff --git a/core/src/test/java/org/apache/gravitino/storage/relational/service/TestGroupMetaService.java b/core/src/test/java/org/apache/gravitino/storage/relational/service/TestGroupMetaService.java index 77cd9d110bc..5e90f0eb89f 100644 --- a/core/src/test/java/org/apache/gravitino/storage/relational/service/TestGroupMetaService.java +++ b/core/src/test/java/org/apache/gravitino/storage/relational/service/TestGroupMetaService.java @@ -27,6 +27,7 @@ import java.sql.SQLException; import java.sql.Statement; import java.time.Instant; +import java.util.Collections; import java.util.Comparator; import java.util.List; import java.util.Optional; @@ -189,6 +190,33 @@ void testListGroups() throws IOException { } } } + + // ISSUE-6061: Test listGroupsByNamespace with revoked users + Function revokeUpdater = + group -> { + AuditInfo updateAuditInfo = + AuditInfo.builder() + .withCreator(group.auditInfo().creator()) + .withCreateTime(group.auditInfo().createTime()) + .withLastModifier("revokeGroup") + .withLastModifiedTime(Instant.now()) + .build(); + + return GroupEntity.builder() + .withNamespace(group.namespace()) + .withId(group.id()) + .withName(group.name()) + .withRoleNames(Collections.emptyList()) + .withRoleIds(Collections.emptyList()) + .withAuditInfo(updateAuditInfo) + .build(); + }; + + Assertions.assertNotNull(groupMetaService.updateGroup(group2.nameIdentifier(), revokeUpdater)); + actualGroups = + groupMetaService.listGroupsByNamespace( + AuthorizationUtils.ofGroupNamespace(metalakeName), true); + Assertions.assertEquals(expectGroups.size(), actualGroups.size()); } @Test diff --git a/core/src/test/java/org/apache/gravitino/storage/relational/service/TestUserMetaService.java b/core/src/test/java/org/apache/gravitino/storage/relational/service/TestUserMetaService.java index 0efd886ee4d..e93a83bafd6 100644 --- a/core/src/test/java/org/apache/gravitino/storage/relational/service/TestUserMetaService.java +++ b/core/src/test/java/org/apache/gravitino/storage/relational/service/TestUserMetaService.java @@ -27,6 +27,7 @@ import java.sql.SQLException; import java.sql.Statement; import java.time.Instant; +import java.util.Collections; import java.util.Comparator; import java.util.List; import java.util.Optional; @@ -188,6 +189,33 @@ void testListUsers() throws IOException { } } } + + // ISSUE-6061: Test listUsersByNamespace with revoked users + Function revokeUpdater = + user -> { + AuditInfo updateAuditInfo = + AuditInfo.builder() + .withCreator(user.auditInfo().creator()) + .withCreateTime(user.auditInfo().createTime()) + .withLastModifier("revokeUser") + .withLastModifiedTime(Instant.now()) + .build(); + + return UserEntity.builder() + .withNamespace(user.namespace()) + .withId(user.id()) + .withName(user.name()) + .withRoleNames(Collections.emptyList()) + .withRoleIds(Collections.emptyList()) + .withAuditInfo(updateAuditInfo) + .build(); + }; + + Assertions.assertNotNull(userMetaService.updateUser(user1.nameIdentifier(), revokeUpdater)); + actualUsers = + userMetaService.listUsersByNamespace( + AuthorizationUtils.ofUserNamespace(metalakeName), true); + Assertions.assertEquals(expectUsers.size(), actualUsers.size()); } @Test