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 <binbin.zh...@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