From 0a77f67785c0917a009116d68139a5e5f9023dbb Mon Sep 17 00:00:00 2001 From: Chris Knoll Date: Fri, 27 Sep 2024 11:46:46 -0400 Subject: [PATCH] Added code to handle test context reloads. Fixed permission tests. --- .../org/ohdsi/webapi/cache/CacheService.java | 6 +- .../service/CohortDefinitionService.java | 10 ++- .../webapi/service/ConceptSetService.java | 16 ++-- .../webapi/service/VocabularyService.java | 34 +++++--- .../ohdsi/webapi/shiro/PermissionManager.java | 80 +++++++++++-------- .../org/ohdsi/webapi/util/CacheHelper.java | 8 ++ .../ohdsi/webapi/security/PermissionTest.java | 4 +- .../resources/permission/permsTest_PREP.json | 42 +++++----- .../permission/wildcardTest_PREP.json | 18 ++--- 9 files changed, 128 insertions(+), 90 deletions(-) diff --git a/src/main/java/org/ohdsi/webapi/cache/CacheService.java b/src/main/java/org/ohdsi/webapi/cache/CacheService.java index f58107725..86875fbd5 100644 --- a/src/main/java/org/ohdsi/webapi/cache/CacheService.java +++ b/src/main/java/org/ohdsi/webapi/cache/CacheService.java @@ -17,7 +17,6 @@ import java.util.ArrayList; import java.util.List; -import java.util.Optional; import java.util.stream.StreamSupport; import javax.cache.Cache; import javax.cache.CacheManager; @@ -54,6 +53,10 @@ public CacheService(CacheManager cacheManager) { this.cacheManager = cacheManager; } + public CacheService() { + } + + @GET @Path("/") @Produces(MediaType.APPLICATION_JSON) @@ -88,5 +91,4 @@ public ClearCacheResult clearAll() { } return result; } - } diff --git a/src/main/java/org/ohdsi/webapi/service/CohortDefinitionService.java b/src/main/java/org/ohdsi/webapi/service/CohortDefinitionService.java index 92caf3420..8b47dd117 100644 --- a/src/main/java/org/ohdsi/webapi/service/CohortDefinitionService.java +++ b/src/main/java/org/ohdsi/webapi/service/CohortDefinitionService.java @@ -163,10 +163,12 @@ public static class CachingSetup implements JCacheManagerCustomizer { @Override public void customize(CacheManager cacheManager) { // Evict when a cohort definition is created or updated, or permissions, or tags - cacheManager.createCache(COHORT_DEFINITION_LIST_CACHE, new MutableConfiguration>() - .setTypes(String.class, (Class>) (Class) List.class) - .setStoreByValue(false) - .setStatisticsEnabled(true)); + if (!CacheHelper.getCacheNames(cacheManager).contains(COHORT_DEFINITION_LIST_CACHE)) { + cacheManager.createCache(COHORT_DEFINITION_LIST_CACHE, new MutableConfiguration>() + .setTypes(String.class, (Class>) (Class) List.class) + .setStoreByValue(false) + .setStatisticsEnabled(true)); + } } } diff --git a/src/main/java/org/ohdsi/webapi/service/ConceptSetService.java b/src/main/java/org/ohdsi/webapi/service/ConceptSetService.java index 9055ef172..4381cdbbd 100644 --- a/src/main/java/org/ohdsi/webapi/service/ConceptSetService.java +++ b/src/main/java/org/ohdsi/webapi/service/ConceptSetService.java @@ -41,6 +41,7 @@ import org.ohdsi.webapi.conceptset.dto.ConceptSetVersionFullDTO; import org.ohdsi.webapi.exception.ConceptNotExistException; import org.ohdsi.webapi.security.PermissionService; +import static org.ohdsi.webapi.service.CohortDefinitionService.CachingSetup.COHORT_DEFINITION_LIST_CACHE; import org.ohdsi.webapi.service.dto.ConceptSetDTO; import org.ohdsi.webapi.shiro.Entities.UserEntity; import org.ohdsi.webapi.shiro.Entities.UserRepository; @@ -51,6 +52,7 @@ import org.ohdsi.webapi.source.SourceService; import org.ohdsi.webapi.tag.domain.HasTags; import org.ohdsi.webapi.tag.dto.TagNameListRequestDTO; +import org.ohdsi.webapi.util.CacheHelper; import org.ohdsi.webapi.util.ExportUtil; import org.ohdsi.webapi.util.NameUtils; import org.ohdsi.webapi.util.ExceptionUtils; @@ -88,14 +90,18 @@ public static class CachingSetup implements JCacheManagerCustomizer { @Override public void customize(CacheManager cacheManager) { + // due to unit tests causing application contexts to reload cache manager caches, we + // have to check for the existance of a cache before creating it + Set cacheNames = CacheHelper.getCacheNames(cacheManager); // Evict when a cohort definition is created or updated, or permissions, or tags - cacheManager.createCache(CONCEPT_SET_LIST_CACHE, new MutableConfiguration>() - .setTypes(String.class, (Class>) (Class) List.class) - .setStoreByValue(false) - .setStatisticsEnabled(true)); + if (!cacheNames.contains(CONCEPT_SET_LIST_CACHE)) { + cacheManager.createCache(CONCEPT_SET_LIST_CACHE, new MutableConfiguration>() + .setTypes(String.class, (Class>) (Class) List.class) + .setStoreByValue(false) + .setStatisticsEnabled(true)); + } } } - @Autowired private ConceptSetGenerationInfoRepository conceptSetGenerationInfoRepository; diff --git a/src/main/java/org/ohdsi/webapi/service/VocabularyService.java b/src/main/java/org/ohdsi/webapi/service/VocabularyService.java index f4523a2e1..7a418de59 100644 --- a/src/main/java/org/ohdsi/webapi/service/VocabularyService.java +++ b/src/main/java/org/ohdsi/webapi/service/VocabularyService.java @@ -53,6 +53,7 @@ import org.ohdsi.webapi.source.SourceService; import org.ohdsi.webapi.source.SourceDaimon; import org.ohdsi.webapi.source.SourceInfo; +import org.ohdsi.webapi.util.CacheHelper; import org.ohdsi.webapi.util.PreparedSqlRender; import org.ohdsi.webapi.util.PreparedStatementRenderer; import org.ohdsi.webapi.vocabulary.ConceptRecommendedNotInstalledException; @@ -98,19 +99,28 @@ public static class CachingSetup implements JCacheManagerCustomizer { @Override public void customize(CacheManager cacheManager) { + // due to unit tests causing application contexts to reload cache manager caches, we + // have to check for the existance of a cache before creating it + Set cacheNames = CacheHelper.getCacheNames(cacheManager); // Evict when a cohort definition is created or updated, or permissions, or tags - cacheManager.createCache(CONCEPT_DETAIL_CACHE, new MutableConfiguration() - .setTypes(String.class, Concept.class) - .setStoreByValue(false) - .setStatisticsEnabled(true)); - cacheManager.createCache(CONCEPT_RELATED_CACHE, new MutableConfiguration>() - .setTypes(String.class, (Class>) (Class) Collection.class) - .setStoreByValue(false) - .setStatisticsEnabled(true)); - cacheManager.createCache(CONCEPT_HIERARCHY_CACHE, new MutableConfiguration>() - .setTypes(String.class, (Class>) (Class) Collection.class) - .setStoreByValue(false) - .setStatisticsEnabled(true)); + if (!cacheNames.contains(CONCEPT_DETAIL_CACHE)) { + cacheManager.createCache(CONCEPT_DETAIL_CACHE, new MutableConfiguration() + .setTypes(String.class, Concept.class) + .setStoreByValue(false) + .setStatisticsEnabled(true)); + } + if (!cacheNames.contains(CONCEPT_RELATED_CACHE)) { + cacheManager.createCache(CONCEPT_RELATED_CACHE, new MutableConfiguration>() + .setTypes(String.class, (Class>) (Class) Collection.class) + .setStoreByValue(false) + .setStatisticsEnabled(true)); + } + if (!cacheNames.contains(CONCEPT_HIERARCHY_CACHE)) { + cacheManager.createCache(CONCEPT_HIERARCHY_CACHE, new MutableConfiguration>() + .setTypes(String.class, (Class>) (Class) Collection.class) + .setStoreByValue(false) + .setStatisticsEnabled(true)); + } } } diff --git a/src/main/java/org/ohdsi/webapi/shiro/PermissionManager.java b/src/main/java/org/ohdsi/webapi/shiro/PermissionManager.java index 13256af83..10c345c59 100644 --- a/src/main/java/org/ohdsi/webapi/shiro/PermissionManager.java +++ b/src/main/java/org/ohdsi/webapi/shiro/PermissionManager.java @@ -41,6 +41,7 @@ import org.apache.shiro.authz.Permission; import org.apache.shiro.authz.permission.WildcardPermission; import org.ohdsi.circe.helper.ResourceHelper; +import org.ohdsi.webapi.util.CacheHelper; import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.cache.JCacheManagerCustomizer; import org.springframework.cache.annotation.CacheEvict; @@ -62,14 +63,18 @@ public static class CachingSetup implements JCacheManagerCustomizer { @Override public void customize(CacheManager cacheManager) { + // due to unit tests causing application contexts to reload cache manager caches, we + // have to check for the existance of a cache before creating it + Set cacheNames = CacheHelper.getCacheNames(cacheManager); // Evict when a user, role or permission is modified/deleted. - cacheManager.createCache(AUTH_INFO_CACHE, new MutableConfiguration() - .setTypes(String.class, UserSimpleAuthorizationInfo.class) - .setStoreByValue(false) - .setStatisticsEnabled(true)); + if (!cacheNames.contains(AUTH_INFO_CACHE)) { + cacheManager.createCache(AUTH_INFO_CACHE, new MutableConfiguration() + .setTypes(String.class, UserSimpleAuthorizationInfo.class) + .setStoreByValue(false) + .setStatisticsEnabled(true)); + } } } - @Value("${datasource.ohdsi.schema}") private String ohdsiSchema; @@ -95,6 +100,8 @@ public void customize(CacheManager cacheManager) { @Autowired private JdbcTemplate jdbcTemplate; + private ThreadLocal> authorizationInfoCache = ThreadLocal.withInitial(ConcurrentHashMap::new); + public static class PermissionsDTO { public Map> permissions = null; @@ -163,39 +170,42 @@ public Iterable getRoles(boolean includePersonalRoles) { @Cacheable(cacheNames = CachingSetup.AUTH_INFO_CACHE) public UserSimpleAuthorizationInfo getAuthorizationInfo(final String login) { - final UserSimpleAuthorizationInfo info = new UserSimpleAuthorizationInfo(); - - final UserEntity userEntity = userRepository.findByLogin(login); - if(userEntity == null) { - throw new UnknownAccountException("Account does not exist"); - } - - info.setUserId(userEntity.getId()); - info.setLogin(userEntity.getLogin()); - - for (UserRoleEntity userRole: userEntity.getUserRoles()) { - info.addRole(userRole.getRole().getName()); - } - - // convert permission index from queryUserPermissions() into a map of WildcardPermissions - Map> permsIdx = this.queryUserPermissions(login).permissions; - Map permissionMap = new HashMap>(); - Set permissionNames = new HashSet<>(); - - for(String permIdxKey : permsIdx.keySet()) { - List perms = permsIdx.get(permIdxKey); - permissionNames.addAll(perms); - // convert raw string permission into Wildcard perm for each element in this key's array. - permissionMap.put(permIdxKey, perms.stream().map(perm -> new WildcardPermission(perm)).collect(Collectors.toList())); - } - - info.setStringPermissions(permissionNames); - info.setPermissionIdx(permissionMap); - return info; - } + return authorizationInfoCache.get().computeIfAbsent(login, newLogin -> { + final UserSimpleAuthorizationInfo info = new UserSimpleAuthorizationInfo(); + + final UserEntity userEntity = userRepository.findByLogin(login); + if(userEntity == null) { + throw new UnknownAccountException("Account does not exist"); + } + + info.setUserId(userEntity.getId()); + info.setLogin(userEntity.getLogin()); + + for (UserRoleEntity userRole: userEntity.getUserRoles()) { + info.addRole(userRole.getRole().getName()); + } + + // convert permission index from queryUserPermissions() into a map of WildcardPermissions + Map> permsIdx = this.queryUserPermissions(login).permissions; + Map permissionMap = new HashMap>(); + Set permissionNames = new HashSet<>(); + + for(String permIdxKey : permsIdx.keySet()) { + List perms = permsIdx.get(permIdxKey); + permissionNames.addAll(perms); + // convert raw string permission into Wildcard perm for each element in this key's array. + permissionMap.put(permIdxKey, perms.stream().map(perm -> new WildcardPermission(perm)).collect(Collectors.toList())); + } + + info.setStringPermissions(permissionNames); + info.setPermissionIdx(permissionMap); + return info; + }); + } @CacheEvict(cacheNames = CachingSetup.AUTH_INFO_CACHE, allEntries = true) public void clearAuthorizationInfoCache() { + authorizationInfoCache.remove(); } @Transactional diff --git a/src/main/java/org/ohdsi/webapi/util/CacheHelper.java b/src/main/java/org/ohdsi/webapi/util/CacheHelper.java index 581dbe14a..6c258f149 100644 --- a/src/main/java/org/ohdsi/webapi/util/CacheHelper.java +++ b/src/main/java/org/ohdsi/webapi/util/CacheHelper.java @@ -1,8 +1,10 @@ package org.ohdsi.webapi.util; import java.lang.management.ManagementFactory; +import java.util.HashSet; import java.util.Set; import javax.cache.CacheManager; +import javax.cache.configuration.MutableConfiguration; import javax.cache.management.CacheStatisticsMXBean; import javax.management.MBeanServer; import javax.management.MBeanServerInvocationHandler; @@ -33,4 +35,10 @@ public static CacheStatisticsMXBean getCacheStats(CacheManager cacheManager, Str throw new RuntimeException(e); } } + + public static Set getCacheNames(CacheManager cacheManager) { + Set cacheNames = new HashSet<>(); + cacheManager.getCacheNames().forEach((name) -> cacheNames.add(name)); + return cacheNames; + } } diff --git a/src/test/java/org/ohdsi/webapi/security/PermissionTest.java b/src/test/java/org/ohdsi/webapi/security/PermissionTest.java index 1aedd9a44..dddcbe2bf 100644 --- a/src/test/java/org/ohdsi/webapi/security/PermissionTest.java +++ b/src/test/java/org/ohdsi/webapi/security/PermissionTest.java @@ -65,7 +65,7 @@ public void setup() { ThreadContext.bind(subject); } - @Ignore +// @Ignore @Test public void permsTest() throws Exception { // need to clear authorization cache before each test @@ -88,7 +88,7 @@ public void permsTest() throws Exception { } - @Ignore +// @Ignore @Test public void wildcardTest() throws Exception { // need to clear authorization cache before each test diff --git a/src/test/resources/permission/permsTest_PREP.json b/src/test/resources/permission/permsTest_PREP.json index 8ad261396..dd5ce2871 100644 --- a/src/test/resources/permission/permsTest_PREP.json +++ b/src/test/resources/permission/permsTest_PREP.json @@ -1,7 +1,7 @@ { "public.sec_user": [ { - "id":1001, + "id":100001, "login":"permsTest", "name":"Perms Test User", "origin":"SYSTEM" @@ -9,56 +9,56 @@ ], "public.sec_role": [ { - "id": 1001, + "id": 100001, "name":"permsTest", "system_role":false } ], "public.sec_permission" : [ { - "id": 1001, + "id": 100001, "value":"printer:manage:printer1" }, { - "id": 1002, + "id": 100002, "value":"printer:manage:printer2" }, { - "id": 1003, + "id": 100003, "value":"printer:query:*" }, { - "id": 1004, + "id": 100004, "value":"printer:print" } ], "public.sec_role_permission" : [ { - "id":1001, - "role_id":1001, - "permission_id":1001 + "id":100001, + "role_id":100001, + "permission_id":100001 }, { - "id":1002, - "role_id":1001, - "permission_id":1002 + "id":100002, + "role_id":100001, + "permission_id":100002 }, { - "id":1003, - "role_id":1001, - "permission_id":1003 + "id":103, + "role_id":100001, + "permission_id":100003 }, { - "id":1004, - "role_id":1001, - "permission_id":1004 + "id":100004, + "role_id":100001, + "permission_id":100004 } ], "public.sec_user_role": [ { - "id": 1001, - "user_id":1001, - "role_id":1001, + "id": 100001, + "user_id":100001, + "role_id":100001, "origin":"SYSTEM" } ] diff --git a/src/test/resources/permission/wildcardTest_PREP.json b/src/test/resources/permission/wildcardTest_PREP.json index 6f08990e8..716ccbb4d 100644 --- a/src/test/resources/permission/wildcardTest_PREP.json +++ b/src/test/resources/permission/wildcardTest_PREP.json @@ -1,7 +1,7 @@ { "public.sec_user": [ { - "id":1001, + "id":100001, "login":"permsTest", "name":"Perms Test User", "origin":"SYSTEM" @@ -9,29 +9,29 @@ ], "public.sec_role": [ { - "id": 1001, + "id": 100001, "name":"permsTest", "system_role":false } ], "public.sec_permission" : [ { - "id": 1001, + "id": 100001, "value":"*" } ], "public.sec_role_permission" : [ { - "id":1001, - "role_id":1001, - "permission_id":1001 + "id":100001, + "role_id":100001, + "permission_id":100001 } ], "public.sec_user_role": [ { - "id": 1001, - "user_id":1001, - "role_id":1001, + "id": 100001, + "user_id":100001, + "role_id":100001, "origin":"SYSTEM" } ]