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();
