This is an automated email from the ASF dual-hosted git repository. xxyu pushed a commit to branch kylin5 in repository https://gitbox.apache.org/repos/asf/kylin.git
commit e96a233e082bd77d0ec30c8bd77f46d14656ae12 Author: Zhiting Guo <35057824+fre...@users.noreply.github.com> AuthorDate: Mon May 15 16:06:20 2023 +0800 KYLIN-5688 allows administrators to inherit data permissions from user groups --------- Co-authored-by: Zhiting Guo <zhiting....@kyligence.io> --- .../apache/kylin/rest/service/AccessService.java | 12 +++-- .../apache/kylin/rest/service/UserAclService.java | 11 ---- .../rest/service/UserAclServiceSupporter.java | 2 - .../org/apache/kylin/rest/util/AclEvaluate.java | 1 - .../kylin/rest/service/AccessServiceTest.java | 60 +++++++++++++--------- .../kylin/rest/service/UserAclServiceTest.java | 8 ++- .../java/org/apache/kylin/rest/util/AclUtil.java | 18 +++++++ .../kylin/rest/service/ProjectServiceTest.java | 1 - .../kylin/rest/service/TableServiceTest.java | 13 +++-- 9 files changed, 77 insertions(+), 49 deletions(-) diff --git a/src/common-service/src/main/java/org/apache/kylin/rest/service/AccessService.java b/src/common-service/src/main/java/org/apache/kylin/rest/service/AccessService.java index fcf1925fad..9e8da0d29b 100644 --- a/src/common-service/src/main/java/org/apache/kylin/rest/service/AccessService.java +++ b/src/common-service/src/main/java/org/apache/kylin/rest/service/AccessService.java @@ -742,7 +742,11 @@ public class AccessService extends BasicService { private Map<Sid, Set<Integer>> getProjectExtPermissions(String projectUuid) { Map<Sid, Set<Integer>> sidWithPermissions = new HashMap<>(); AclEntity ae = getAclEntity(AclEntityType.PROJECT_INSTANCE, projectUuid); - AclRecord aclRecord = getAcl(ae).getAclRecord(); + MutableAclRecord acl = getAcl(ae); + if (acl == null) { + return sidWithPermissions; + } + AclRecord aclRecord = acl.getAclRecord(); if (aclRecord != null && aclRecord.getEntries() != null) { List<AccessControlEntry> aces = aclRecord.getEntries(); sidWithPermissions = aces.stream().filter(ace -> AclPermissionUtil.hasExtPermission(ace.getPermission())) @@ -762,9 +766,9 @@ public class AccessService extends BasicService { if (userAclService.canAdminUserQuery(userName)) { return Collections.singleton(AclConstants.DATA_QUERY); } - if (userService.isGlobalAdmin(userName)) { - val hasDataQueryPermission = userAclService.hasUserAclPermissionInProject(userName, project); - return hasDataQueryPermission ? Collections.singleton(AclConstants.DATA_QUERY) : Collections.emptySet(); + if (userService.isGlobalAdmin(userName) + && userAclService.hasUserAclPermissionInProject(userName, project)) { + return Collections.singleton(AclConstants.DATA_QUERY); } return getUserNormalExtPermissions(projectUuid, userName).stream() .map(ExternalAclProvider::convertToExternalPermission).collect(Collectors.toSet()); diff --git a/src/common-service/src/main/java/org/apache/kylin/rest/service/UserAclService.java b/src/common-service/src/main/java/org/apache/kylin/rest/service/UserAclService.java index 297fb87afb..03af652c49 100644 --- a/src/common-service/src/main/java/org/apache/kylin/rest/service/UserAclService.java +++ b/src/common-service/src/main/java/org/apache/kylin/rest/service/UserAclService.java @@ -55,7 +55,6 @@ import org.apache.kylin.rest.security.UserAcl; import org.apache.kylin.rest.security.UserAclManager; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Qualifier; -import org.springframework.security.access.AccessDeniedException; import org.springframework.security.acls.model.Permission; import org.springframework.security.core.Authentication; import org.springframework.security.core.authority.SimpleGrantedAuthority; @@ -251,16 +250,6 @@ public class UserAclService extends BasicService implements UserAclServiceSuppor return authentication.getName(); } - @Override - @SneakyThrows(IOException.class) - public void checkAdminUserPermission(String projectName) { - String username = getLoginUsername(); - if (userService.isGlobalAdmin(username) && !hasUserAclPermission(username, AclPermission.DATA_QUERY) - && !hasUserAclPermissionInProject(username, projectName)) { - throw new AccessDeniedException(StringUtils.EMPTY); - } - } - @Transaction public void updateUserAclPermission(UserDetails user, Permission permission) { UserAclManager userAclManager = getManager(UserAclManager.class); diff --git a/src/common-service/src/main/java/org/apache/kylin/rest/service/UserAclServiceSupporter.java b/src/common-service/src/main/java/org/apache/kylin/rest/service/UserAclServiceSupporter.java index 5ef8b32705..091356b08f 100644 --- a/src/common-service/src/main/java/org/apache/kylin/rest/service/UserAclServiceSupporter.java +++ b/src/common-service/src/main/java/org/apache/kylin/rest/service/UserAclServiceSupporter.java @@ -27,7 +27,5 @@ public interface UserAclServiceSupporter { boolean canAdminUserQuery(String username); - void checkAdminUserPermission(String project); - boolean hasUserAclPermissionInProject(String project); } diff --git a/src/common-service/src/main/java/org/apache/kylin/rest/util/AclEvaluate.java b/src/common-service/src/main/java/org/apache/kylin/rest/util/AclEvaluate.java index ff7608a612..b3239a60d3 100644 --- a/src/common-service/src/main/java/org/apache/kylin/rest/util/AclEvaluate.java +++ b/src/common-service/src/main/java/org/apache/kylin/rest/util/AclEvaluate.java @@ -61,7 +61,6 @@ public class AclEvaluate { if (userAclService.canAdminUserQuery() || userAclService.hasUserAclPermissionInProject(projectName)) { return; } - userAclService.checkAdminUserPermission(projectName); aclUtil.hasProjectDataQueryPermission(getProjectInstance(projectName)); } diff --git a/src/common-service/src/test/java/org/apache/kylin/rest/service/AccessServiceTest.java b/src/common-service/src/test/java/org/apache/kylin/rest/service/AccessServiceTest.java index 59f8eafd45..f5402d462c 100644 --- a/src/common-service/src/test/java/org/apache/kylin/rest/service/AccessServiceTest.java +++ b/src/common-service/src/test/java/org/apache/kylin/rest/service/AccessServiceTest.java @@ -46,12 +46,13 @@ import org.apache.kylin.common.exception.KylinException; import org.apache.kylin.common.msg.MsgPicker; import org.apache.kylin.common.persistence.AclEntity; import org.apache.kylin.common.util.NLocalFileMetadataTestCase; +import org.apache.kylin.guava30.shaded.common.collect.Lists; +import org.apache.kylin.guava30.shaded.common.collect.Sets; import org.apache.kylin.metadata.project.NProjectManager; import org.apache.kylin.metadata.project.ProjectInstance; import org.apache.kylin.metadata.user.ManagedUser; import org.apache.kylin.rest.constant.Constant; import org.apache.kylin.rest.request.AccessRequest; -import org.apache.kylin.rest.request.GlobalAccessRequest; import org.apache.kylin.rest.response.AccessEntryResponse; import org.apache.kylin.rest.security.AclEntityType; import org.apache.kylin.rest.security.AclPermission; @@ -67,7 +68,6 @@ import org.apache.kylin.rest.util.SpringContext; import org.junit.After; import org.junit.Assert; import org.junit.Before; -import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.function.ThrowingRunnable; @@ -80,6 +80,7 @@ import org.powermock.api.mockito.PowerMockito; import org.powermock.core.classloader.annotations.PrepareForTest; import org.powermock.modules.junit4.PowerMockRunner; import org.springframework.context.ApplicationContext; +import org.springframework.security.access.AccessDeniedException; import org.springframework.security.acls.domain.BasePermission; import org.springframework.security.acls.domain.GrantedAuthoritySid; import org.springframework.security.acls.domain.PermissionFactory; @@ -95,10 +96,6 @@ import org.springframework.security.core.authority.SimpleGrantedAuthority; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.test.util.ReflectionTestUtils; -import com.fasterxml.jackson.core.JsonProcessingException; -import org.apache.kylin.guava30.shaded.common.collect.Lists; -import org.apache.kylin.guava30.shaded.common.collect.Sets; - @RunWith(PowerMockRunner.class) @PrepareForTest({ SpringContext.class, UserGroupInformation.class, KylinConfig.class, NProjectManager.class }) public class AccessServiceTest extends NLocalFileMetadataTestCase { @@ -144,6 +141,7 @@ public class AccessServiceTest extends NLocalFileMetadataTestCase { SecurityContextHolder.getContext().setAuthentication(authentication); ReflectionTestUtils.setField(aclEvaluate, "aclUtil", aclUtil); + ReflectionTestUtils.setField(aclEvaluate, "userAclService", userAclService); ReflectionTestUtils.setField(userAclService, "userService", userService); getTestConfig().setProperty("kylin.security.acl.data-permission-default-enabled", "false"); @@ -390,22 +388,6 @@ public class AccessServiceTest extends NLocalFileMetadataTestCase { return request; } - @Ignore("just ignore") - @Test - public void test100000Entries() throws JsonProcessingException { - AclServiceTest.MockAclEntity ae = new AclServiceTest.MockAclEntity("100000Entries"); - long time = System.currentTimeMillis(); - for (int i = 0; i < 100000; i++) { - if (i % 10 == 0) { - long now = System.currentTimeMillis(); - System.out.println((now - time) + " ms for last 10 entries, total " + i); - time = now; - } - Sid sid = accessService.getSid("USER" + i, true); - accessService.grant(ae, AclPermission.OPERATION, sid); - } - } - @Test(expected = KylinException.class) public void testCheckGlobalAdminException() throws IOException { accessService.checkGlobalAdmin("ADMIN"); @@ -512,14 +494,42 @@ public class AccessServiceTest extends NLocalFileMetadataTestCase { @Test public void testAdminUserExtPermissionInProject() { + // super admin assertTrue(accessService.getUserNormalExtPermissions("default").contains("DATA_QUERY")); - GlobalAccessRequest globalAccessRequest = new GlobalAccessRequest(); - globalAccessRequest.setUsername("ADMIN"); - globalAccessRequest.setProject("default"); + aclEvaluate.checkProjectQueryPermission("default"); + + // set admin as a non-super admin Mockito.when(userService.listSuperAdminUsers()).thenReturn(Collections.emptyList()); + + // system admin without data query permission + assertFalse(accessService.getUserNormalExtPermissions("default").contains("DATA_QUERY")); + Assert.assertThrows(AccessDeniedException.class, ()-> aclEvaluate.checkProjectQueryPermission("default")); + + // system admin with global data query permission + userAclService.getManager(UserAclManager.class).addPermission("ADMIN", AclPermission.DATA_QUERY); + assertTrue(accessService.getUserNormalExtPermissions("default").contains("DATA_QUERY")); + aclEvaluate.checkProjectQueryPermission("default"); + userAclService.getManager(UserAclManager.class).deletePermission("ADMIN", AclPermission.DATA_QUERY); + + // system admin with data query permission for special project + assertFalse(accessService.getUserNormalExtPermissions("default").contains("DATA_QUERY")); + userAclService.getManager(UserAclManager.class).addDataQueryProject("ADMIN", "default"); Mockito.when(userAclService.canAdminUserQuery(Mockito.anyString())).thenReturn(false); assertTrue(accessService.getUserNormalExtPermissions("default").contains("DATA_QUERY")); + aclEvaluate.checkProjectQueryPermission("default"); + userAclService.getManager(UserAclManager.class).deleteDataQueryProject("ADMIN", "default"); + + // system admin with group which has query permission for special project + assertFalse(accessService.getUserNormalExtPermissions("default").contains("DATA_QUERY")); + + String projectUuid = NProjectManager.getInstance(KylinConfig.getInstanceFromEnv()).getProject("default") + .getUuid(); + AclEntity ae = accessService.getAclEntity(AclEntityType.PROJECT_INSTANCE, projectUuid); + getTestConfig().setProperty("kylin.security.acl.data-permission-default-enabled", "true"); + accessService.grant(ae, AclPermission.READ, new GrantedAuthoritySid("ROLE_ADMIN")); + assertTrue(accessService.getUserNormalExtPermissions("default").contains("DATA_QUERY")); + aclEvaluate.checkProjectQueryPermission("default"); } @Test diff --git a/src/common-service/src/test/java/org/apache/kylin/rest/service/UserAclServiceTest.java b/src/common-service/src/test/java/org/apache/kylin/rest/service/UserAclServiceTest.java index c1c67df8a5..3cc15a082e 100644 --- a/src/common-service/src/test/java/org/apache/kylin/rest/service/UserAclServiceTest.java +++ b/src/common-service/src/test/java/org/apache/kylin/rest/service/UserAclServiceTest.java @@ -37,6 +37,7 @@ import org.apache.kylin.rest.request.GlobalBatchAccessRequest; import org.apache.kylin.rest.security.AclPermission; import org.apache.kylin.rest.security.UserAclManager; import org.apache.kylin.rest.util.AclEvaluate; +import org.apache.kylin.rest.util.AclUtil; import org.junit.Assert; import org.junit.Before; import org.junit.Rule; @@ -44,6 +45,7 @@ import org.junit.Test; import org.junit.rules.ExpectedException; import org.mockito.Mock; import org.mockito.Mockito; +import org.mockito.Spy; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.security.access.AccessDeniedException; @@ -67,6 +69,9 @@ public class UserAclServiceTest extends ServiceTestBase { @Mock AclEvaluate aclEvaluate = Mockito.spy(new AclEvaluate()); + @Spy + AclUtil aclUtil = Mockito.spy(new AclUtil()); + @Rule public ExpectedException thrown = ExpectedException.none(); @@ -76,6 +81,7 @@ public class UserAclServiceTest extends ServiceTestBase { getTestConfig().setProperty("kylin.security.acl.data-permission-default-enabled", "true"); ReflectionTestUtils.setField(userAclService, "userService", userService); ReflectionTestUtils.setField(aclEvaluate, "userAclService", userAclService); + ReflectionTestUtils.setField(aclEvaluate, "aclUtil", aclUtil); } @Test @@ -180,8 +186,6 @@ public class UserAclServiceTest extends ServiceTestBase { Assert.assertThrows(MsgPicker.getMsg().getGrantPermissionFailedByIllegalAuthorizingUser(), KylinException.class, () -> ReflectionTestUtils.invokeMethod(userAclService, "checkLoginUserPermissionInPrj", "default")); Assert.assertFalse(userAclService.hasUserAclPermissionInProject("default")); - Assert.assertThrows(AccessDeniedException.class, - () -> ReflectionTestUtils.invokeMethod(userAclService, "checkAdminUserPermission", "default")); Assert.assertThrows(AccessDeniedException.class, () -> ReflectionTestUtils.invokeMethod(aclEvaluate, "checkProjectQueryPermission", "default")); } diff --git a/src/core-metadata/src/main/java/org/apache/kylin/rest/util/AclUtil.java b/src/core-metadata/src/main/java/org/apache/kylin/rest/util/AclUtil.java index 70ced83f3d..ca01d4ed13 100644 --- a/src/core-metadata/src/main/java/org/apache/kylin/rest/util/AclUtil.java +++ b/src/core-metadata/src/main/java/org/apache/kylin/rest/util/AclUtil.java @@ -18,13 +18,22 @@ package org.apache.kylin.rest.util; +import org.apache.kylin.common.KylinConfig; import org.apache.kylin.metadata.project.ProjectInstance; import org.apache.kylin.rest.constant.Constant; +import org.apache.kylin.rest.security.AclPermission; +import org.apache.kylin.rest.security.AclPermissionFactory; +import org.apache.kylin.rest.security.KylinAclPermissionEvaluator; +import org.apache.kylin.rest.service.AclService; import org.springframework.context.annotation.Lazy; +import org.springframework.security.access.AccessDeniedException; import org.springframework.security.access.prepost.PreAuthorize; +import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.stereotype.Component; +import com.alibaba.nacos.api.utils.StringUtils; + @Lazy @Component("aclUtil") public class AclUtil { @@ -41,6 +50,15 @@ public class AclUtil { @PreAuthorize(Constant.ACCESS_POST_FILTER_READ_FOR_DATA_PERMISSION_SEPARATE) public boolean hasProjectDataQueryPermission(ProjectInstance project) { + if (KylinConfig.getInstanceFromEnv().isUTEnv()) { + // PreAuthorize does not work in UT. So let's make an equivalent implementation for it. + Authentication auth = SecurityContextHolder.getContext().getAuthentication(); + KylinAclPermissionEvaluator evaluator = new KylinAclPermissionEvaluator(new AclService(), + new AclPermissionFactory()); + if (!evaluator.hasPermission(auth, project, AclPermission.DATA_QUERY)) { + throw new AccessDeniedException(StringUtils.EMPTY); + } + } return true; } diff --git a/src/modeling-service/src/test/java/org/apache/kylin/rest/service/ProjectServiceTest.java b/src/modeling-service/src/test/java/org/apache/kylin/rest/service/ProjectServiceTest.java index 10a1c9a66b..234b370e71 100644 --- a/src/modeling-service/src/test/java/org/apache/kylin/rest/service/ProjectServiceTest.java +++ b/src/modeling-service/src/test/java/org/apache/kylin/rest/service/ProjectServiceTest.java @@ -1010,7 +1010,6 @@ public class ProjectServiceTest extends NLocalFileMetadataTestCase { @Test public void testHasProjectAdminPermission() { Assert.assertTrue(aclEvaluate.hasProjectAdminPermission(PROJECT)); - aclEvaluate.checkProjectQueryPermission(PROJECT); } @Test diff --git a/src/modeling-service/src/test/java/org/apache/kylin/rest/service/TableServiceTest.java b/src/modeling-service/src/test/java/org/apache/kylin/rest/service/TableServiceTest.java index a04a0b7f71..14b835de6c 100644 --- a/src/modeling-service/src/test/java/org/apache/kylin/rest/service/TableServiceTest.java +++ b/src/modeling-service/src/test/java/org/apache/kylin/rest/service/TableServiceTest.java @@ -22,6 +22,7 @@ import static org.apache.kylin.common.exception.code.ErrorCodeServer.MODEL_ID_NO import static org.apache.kylin.common.exception.code.ErrorCodeServer.MODEL_NAME_NOT_EXIST; import static org.apache.kylin.metadata.model.NTableMetadataManager.getInstance; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.when; import java.io.DataInputStream; @@ -57,6 +58,9 @@ import org.apache.kylin.common.scheduler.EventBusFactory; import org.apache.kylin.common.util.CliCommandExecutor; import org.apache.kylin.common.util.Pair; import org.apache.kylin.common.util.S3AUtil; +import org.apache.kylin.guava30.shaded.common.collect.Lists; +import org.apache.kylin.guava30.shaded.common.collect.Maps; +import org.apache.kylin.guava30.shaded.common.collect.Sets; import org.apache.kylin.job.constant.JobStatusEnum; import org.apache.kylin.metadata.acl.AclTCR; import org.apache.kylin.metadata.acl.AclTCRManager; @@ -76,6 +80,7 @@ import org.apache.kylin.metadata.project.ProjectInstance; import org.apache.kylin.metadata.realization.RealizationStatusEnum; import org.apache.kylin.metadata.recommendation.candidate.JdbcRawRecStore; import org.apache.kylin.metadata.streaming.KafkaConfig; +import org.apache.kylin.metadata.user.ManagedUser; import org.apache.kylin.query.util.PushDownUtil; import org.apache.kylin.rest.constant.Constant; import org.apache.kylin.rest.request.AutoMergeRequest; @@ -110,9 +115,6 @@ import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.test.util.ReflectionTestUtils; import com.fasterxml.jackson.databind.ObjectMapper; -import org.apache.kylin.guava30.shaded.common.collect.Lists; -import org.apache.kylin.guava30.shaded.common.collect.Maps; -import org.apache.kylin.guava30.shaded.common.collect.Sets; import lombok.val; import lombok.var; @@ -154,6 +156,9 @@ public class TableServiceTest extends CSVSourceTestCase { @Mock private KafkaService kafkaServiceMock = Mockito.mock(KafkaService.class); + @Mock + private AclService aclService = Mockito.mock(AclService.class); + @InjectMocks private FusionModelService fusionModelService = Mockito.spy(new FusionModelService()); @@ -177,6 +182,7 @@ public class TableServiceTest extends CSVSourceTestCase { ReflectionTestUtils.setField(userAclService, "userService", userService); ReflectionTestUtils.setField(accessService, "userAclService", userAclService); ReflectionTestUtils.setField(accessService, "userService", userService); + ReflectionTestUtils.setField(accessService, "aclService", aclService); ReflectionTestUtils.setField(tableService, "accessService", accessService); NProjectManager projectManager = NProjectManager.getInstance(KylinConfig.getInstanceFromEnv()); ProjectInstance projectInstance = projectManager.getProject("default"); @@ -328,6 +334,7 @@ public class TableServiceTest extends CSVSourceTestCase { Mockito.when(userService.isGlobalAdmin(Mockito.anyString())).thenReturn(true); Mockito.when(userAclService.hasUserAclPermissionInProject(Mockito.anyString(), Mockito.anyString())) .thenReturn(false); + Mockito.when(userService.loadUserByUsername(eq("test"))).thenReturn(new ManagedUser()); List<TableDesc> tableExtList = tableService .getTableDesc("newten", true, "TEST_COUNTRY", "DEFAULT", true, Collections.emptyList(), 10).getFirst();