You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kylin.apache.org by xx...@apache.org on 2022/12/28 08:01:08 UTC
[kylin] branch kylin5 updated: KYLIN-5373 fix user/usergroup display error in project acl
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
The following commit(s) were added to refs/heads/kylin5 by this push:
new 2f010f808a KYLIN-5373 fix user/usergroup display error in project acl
2f010f808a is described below
commit 2f010f808a3c0ab5f7d3c9ad82805c2845d2434e
Author: binbin.zheng <bi...@kyligence.io>
AuthorDate: Wed Oct 26 18:56:30 2022 +0800
KYLIN-5373 fix user/usergroup display error in project acl
---
.../apache/kylin/rest/service/AccessService.java | 30 +++++++++++++++++++---
.../apache/kylin/rest/service/ProjectService.java | 19 +++++++++++---
.../kylin/rest/service/AccessServiceTest.java | 26 +++++++++++++++++--
.../kylin/rest/service/UserAclServiceTest.java | 4 ---
4 files changed, 67 insertions(+), 12 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 9c98337960..cd402e0c9f 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
@@ -481,7 +481,7 @@ public class AccessService extends BasicService {
List<AccessEntryResponse> result = new ArrayList<>();
for (AccessControlEntry ace : acl.getEntries()) {
- if (StringUtils.isNotEmpty(nameSeg) && !needAdd(nameSeg, isCaseSensitive, getName(ace.getSid()))) {
+ if (nameSegNotMatch(ace, nameSeg, isCaseSensitive) || sidNotExists(ace)) {
continue;
}
result.add(new AccessEntryResponse(ace.getId(), ace.getSid(), ace.getPermission(), ace.isGranting()));
@@ -490,12 +490,20 @@ public class AccessService extends BasicService {
return result;
}
+ private boolean nameSegNotMatch(AccessControlEntry ace, String nameSeg, boolean isCaseSensitive) {
+ return StringUtils.isNotEmpty(nameSeg) && !needAdd(nameSeg, isCaseSensitive, getName(ace.getSid()));
+ }
+
+ private boolean sidNotExists(AccessControlEntry ace) {
+ return isPrincipalSidNotExists(ace.getSid()) || isGrantedAuthoritySidNotExists(ace.getSid());
+ }
+
private boolean needAdd(String nameSeg, boolean isCaseSensitive, String name) {
return isCaseSensitive && StringUtils.contains(name, nameSeg)
|| !isCaseSensitive && StringUtils.containsIgnoreCase(name, nameSeg);
}
- private static String getName(Sid sid) {
+ public static String getName(Sid sid) {
if (sid instanceof PrincipalSid) {
return ((PrincipalSid) sid).getPrincipal();
} else {
@@ -503,6 +511,19 @@ public class AccessService extends BasicService {
}
}
+ public boolean isPrincipalSidNotExists(Sid sid) {
+ return (sid instanceof PrincipalSid) && !userService.userExists(((PrincipalSid) sid).getPrincipal());
+ }
+
+ public boolean isGrantedAuthoritySidNotExists(Sid sid) {
+ try {
+ return (sid instanceof GrantedAuthoritySid)
+ && !userGroupService.exists(((GrantedAuthoritySid) sid).getGrantedAuthority());
+ } catch (IOException e) {
+ return true;
+ }
+ }
+
public List<AccessEntryResponse> generateAceResponses(Acl acl) {
return generateAceResponsesByFuzzMatching(acl, null, false);
}
@@ -537,13 +558,16 @@ public class AccessService extends BasicService {
List<String> result = new ArrayList<>();
for (AccessControlEntry ace : acl.getEntries()) {
String name = null;
+ boolean notExisted = false;
if (type.equalsIgnoreCase(MetadataConstants.TYPE_USER) && ace.getSid() instanceof PrincipalSid) {
name = ((PrincipalSid) ace.getSid()).getPrincipal();
+ notExisted = isPrincipalSidNotExists(ace.getSid());
}
if (type.equalsIgnoreCase(MetadataConstants.TYPE_GROUP) && ace.getSid() instanceof GrantedAuthoritySid) {
name = ((GrantedAuthoritySid) ace.getSid()).getGrantedAuthority();
+ notExisted = isGrantedAuthoritySidNotExists(ace.getSid());
}
- if (!StringUtils.isBlank(name)) {
+ if (!StringUtils.isBlank(name) && !notExisted) {
result.add(name);
}
}
diff --git a/src/common-service/src/main/java/org/apache/kylin/rest/service/ProjectService.java b/src/common-service/src/main/java/org/apache/kylin/rest/service/ProjectService.java
index ebc3d971c8..10c5b19f6f 100644
--- a/src/common-service/src/main/java/org/apache/kylin/rest/service/ProjectService.java
+++ b/src/common-service/src/main/java/org/apache/kylin/rest/service/ProjectService.java
@@ -78,6 +78,7 @@ import org.apache.kylin.common.util.SetThreadName;
import org.apache.kylin.job.constant.JobStatusEnum;
import org.apache.kylin.job.execution.AbstractExecutable;
import org.apache.kylin.job.execution.NExecutableManager;
+import org.apache.kylin.metadata.MetadataConstants;
import org.apache.kylin.metadata.cube.storage.ProjectStorageInfoCollector;
import org.apache.kylin.metadata.cube.storage.StorageInfoEnum;
import org.apache.kylin.metadata.model.ISourceAware;
@@ -376,14 +377,26 @@ public class ProjectService extends BasicService {
public void cleanupAcl() {
EnhancedUnitOfWork.doInTransactionWithCheckAndRetry(() -> {
val prjManager = NProjectManager.getInstance(KylinConfig.getInstanceFromEnv());
- List<String> prjects = prjManager.listAllProjects().stream().map(ProjectInstance::getUuid)
- .collect(Collectors.toList());
+ Set<String> projects = prjManager.listAllProjects().stream().map(ProjectInstance::getUuid)
+ .collect(Collectors.toSet());
val aclManager = AclManager.getInstance(KylinConfig.getInstanceFromEnv());
for (val acl : aclManager.listAll()) {
String id = acl.getDomainObjectInfo().getId();
- if (!prjects.contains(id)) {
+ if (!projects.contains(id)) {
aclManager.delete(id);
+ continue;
}
+ val aceList = acl.getEntries();
+ aceList.forEach(ace -> {
+ if (accessService.isPrincipalSidNotExists(ace.getSid())) {
+ accessService.revokeProjectPermission(AccessService.getName(ace.getSid()),
+ MetadataConstants.TYPE_USER);
+ }
+ if (accessService.isGrantedAuthoritySidNotExists(ace.getSid())) {
+ accessService.revokeProjectPermission(accessService.getName(ace.getSid()),
+ MetadataConstants.TYPE_GROUP);
+ }
+ });
}
return 0;
}, UnitOfWork.GLOBAL_UNIT);
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 ea76fae534..fb102f63de 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
@@ -428,6 +428,8 @@ public class AccessServiceTest extends NLocalFileMetadataTestCase {
sidToPerm.put(new GrantedAuthoritySid("ROLE_ADMIN"), AclPermission.ADMINISTRATION);
sidToPerm.put(new GrantedAuthoritySid("role_ADMIN"), AclPermission.ADMINISTRATION);
accessService.batchGrant(ae, sidToPerm);
+ Mockito.when(userGroupService.exists(Mockito.anyString())).thenReturn(true);
+ Mockito.when(userService.userExists(Mockito.anyString())).thenReturn(true);
List<AccessEntryResponse> result = accessService.generateAceResponsesByFuzzMatching(ae, "", false);
assertEquals(2, result.size());
assertEquals("ANALYST", ((PrincipalSid) result.get(0).getSid()).getPrincipal());
@@ -437,6 +439,8 @@ public class AccessServiceTest extends NLocalFileMetadataTestCase {
public void testGenerateAceResponsesByFuzzMatchingWhenHasSameNameUserAndGroupName() throws Exception {
AclEntity ae = new AclServiceTest.MockAclEntity("test");
final Map<Sid, Permission> sidToPerm = new HashMap<>();
+ Mockito.when(userGroupService.exists("ADMIN")).thenReturn(true);
+ sidToPerm.put(new GrantedAuthoritySid("grp1"), AclPermission.ADMINISTRATION);
sidToPerm.put(new GrantedAuthoritySid("ADMIN"), AclPermission.ADMINISTRATION);
sidToPerm.put(new PrincipalSid("ADMIN"), AclPermission.ADMINISTRATION);
accessService.batchGrant(ae, sidToPerm);
@@ -471,6 +475,21 @@ public class AccessServiceTest extends NLocalFileMetadataTestCase {
.setAuthentication(new TestingAuthenticationToken("ADMIN", "ADMIN", Constant.ROLE_ADMIN));
}
+ @Test
+ public void testCleanupProjectAcl() throws Exception {
+ AclEntity ae = new AclServiceTest.MockAclEntity("test");
+ final Map<Sid, Permission> sidToPerm = new HashMap<>();
+ sidToPerm.put(new PrincipalSid("ADMIN"), AclPermission.ADMINISTRATION);
+ sidToPerm.put(new PrincipalSid("admin"), AclPermission.ADMINISTRATION);
+ sidToPerm.put(new PrincipalSid("ANALYST"), AclPermission.ADMINISTRATION);
+ sidToPerm.put(new GrantedAuthoritySid("ROLE_ADMIN"), AclPermission.ADMINISTRATION);
+ sidToPerm.put(new GrantedAuthoritySid("role_ADMIN"), AclPermission.ADMINISTRATION);
+ accessService.batchGrant(ae, sidToPerm);
+ projectService.cleanupAcl();
+ List<AccessEntryResponse> result = accessService.generateAceResponsesByFuzzMatching(ae, "", false);
+ assertEquals(0, result.size());
+ }
+
@Test
public void testRevokeWithSid() {
AclEntity ae = new AclServiceTest.MockAclEntity("test-domain-object");
@@ -722,7 +741,8 @@ public class AccessServiceTest extends NLocalFileMetadataTestCase {
AclEntity ae = accessService.getAclEntity(AclEntityType.PROJECT_INSTANCE,
"1eaca32a-a33e-4b69-83dd-0bb8b1f8c91b");
Map<String, List<String>> map = accessService.getProjectUsersAndGroups(ae);
- assertEquals(4, map.get("user").size());
+ assertEquals(2, map.get("user").size());
+ assertEquals(1, map.get("group").size());
assertTrue(map.get("user").contains("ADMIN"));
assertTrue(map.get("group").contains("ROLE_ADMIN"));
}
@@ -766,10 +786,12 @@ public class AccessServiceTest extends NLocalFileMetadataTestCase {
}
@Test
- public void testAclWithUnNaturalOrderUpdate() {
+ public void testAclWithUnNaturalOrderUpdate() throws IOException{
AclEntity ae = accessService.getAclEntity(AclEntityType.PROJECT_INSTANCE,
"1eaca32a-a33e-4b69-83dd-0bb8b1f8c91b");
+ Mockito.when(userService.userExists(Mockito.anyString())).thenReturn(true);
+ Mockito.when(userGroupService.exists(Mockito.anyString())).thenReturn(true);
// read from metadata
MutableAclRecord acl = accessService.getAcl(ae);
// order by sid_order in aceImpl
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 35c7e70657..3814e8bc39 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,10 +37,8 @@ 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.SpringContext;
import org.junit.Assert;
import org.junit.Before;
-import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
@@ -120,7 +118,6 @@ public class UserAclServiceTest extends ServiceTestBase {
userAclService.grantUserAclPermission("admin", "DATA_QUERY");
}
- @Ignore("very unstable")
@Test
public void testGetAllUsersHasGlobalPermission() {
KylinUserService kylinUserService = new KylinUserService() {
@@ -131,7 +128,6 @@ public class UserAclServiceTest extends ServiceTestBase {
};
ReflectionTestUtils.setField(userAclService, "userService", kylinUserService);
Assert.assertTrue(userAclService.listUserAcl().isEmpty());
- ReflectionTestUtils.setField(userAclService, "userService", SpringContext.getBean(UserService.class));
}
@Test