From 379d73bbfd09614d980065ed0e29e408e77bbbfa Mon Sep 17 00:00:00 2001 From: Chris Knoll Date: Tue, 27 Feb 2024 16:22:23 -0500 Subject: [PATCH 1/4] Removed role cache from Permission Service. Changed read/write permission checks to use permissions instead of roles. Permission checks are using Subject.isPermitted() which honors wildcard semantics. Altered JwtAuthRealm to filter user permissions to either * or first element of permission to check for speed. Changed permission index from JsonNode to Map<>. Serializes same way, but map semantics are simpler to navigate. Altered AuthrizationInfo to contain index of Permissions and store Wildcard perms. General cleanup of unused imports and removed unused dependencies (ie: Autowired fields were removed if no longer needed). Fixes #2353. --- .../webapi/security/PermissionService.java | 149 ++++-------------- .../model/UserSimpleAuthorizationInfo.java | 42 +++-- .../org/ohdsi/webapi/service/UserService.java | 2 +- .../ohdsi/webapi/shiro/PermissionManager.java | 45 ++++-- .../webapi/shiro/filters/CacheFilter.java | 4 - .../webapi/shiro/realms/JwtAuthRealm.java | 22 ++- 6 files changed, 107 insertions(+), 157 deletions(-) diff --git a/src/main/java/org/ohdsi/webapi/security/PermissionService.java b/src/main/java/org/ohdsi/webapi/security/PermissionService.java index 5bb63729d3..cc510579dd 100644 --- a/src/main/java/org/ohdsi/webapi/security/PermissionService.java +++ b/src/main/java/org/ohdsi/webapi/security/PermissionService.java @@ -4,7 +4,6 @@ import com.cosium.spring.data.jpa.entity.graph.domain.EntityGraphUtils; import org.apache.shiro.authz.UnauthorizedException; import org.ohdsi.webapi.model.CommonEntity; -import org.ohdsi.webapi.security.dto.RoleDTO; import org.ohdsi.webapi.security.model.EntityPermissionSchema; import org.ohdsi.webapi.security.model.EntityPermissionSchemaResolver; import org.ohdsi.webapi.security.model.EntityType; @@ -34,16 +33,15 @@ import javax.annotation.PostConstruct; import java.io.Serializable; import java.util.Arrays; -import java.util.Collection; -import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; -import java.util.function.Function; import java.util.stream.Collectors; +import org.apache.shiro.SecurityUtils; +import org.apache.shiro.authz.Permission; +import org.apache.shiro.authz.permission.WildcardPermission; +import org.apache.shiro.subject.Subject; @Service public class PermissionService { @@ -64,9 +62,6 @@ public class PermissionService { @Value("#{!'${security.provider}'.equals('DisabledSecurity')}") private boolean securityEnabled; - private ThreadLocal>>> permissionCache = - ThreadLocal.withInitial(ConcurrentHashMap::new); - private final EntityGraph PERMISSION_ENTITY_GRAPH = EntityGraphUtils.fromAttributePaths("rolePermissions", "rolePermissions.role"); public PermissionService( @@ -133,14 +128,14 @@ public void checkCommonEntityOwnership(EntityType entityType, Integer entityId) public Map getPermissionTemplates(EntityPermissionSchema permissionSchema, AccessType accessType) { - switch (accessType) { - case WRITE: - return permissionSchema.getWritePermissions(); - case READ: - return permissionSchema.getReadPermissions(); - default: - throw new UnsupportedOperationException(); - } + switch (accessType) { + case WRITE: + return permissionSchema.getWritePermissions(); + case READ: + return permissionSchema.getReadPermissions(); + default: + throw new UnsupportedOperationException(); + } } public List finaAllRolesHavingPermissions(List permissions) { @@ -178,97 +173,28 @@ private boolean isCurrentUserOwnerOf(CommonEntity entity) { return Objects.equals(owner.getLogin(), loggedInUsername); } + public List getEntityPermissions(EntityType entityType, Number id, AccessType accessType) { + Set permissionTemplates = getTemplatesForType(entityType, accessType).keySet(); - public void preparePermissionCache(EntityType entityType, Set permissionTemplates) { - if (permissionCache.get().get(entityType) == null) { - final ConcurrentHashMap> rolesForEntity = new ConcurrentHashMap<>(); - permissionCache.get().put(entityType, rolesForEntity); - - List permissionsSQLTemplates = permissionTemplates.stream() - .map(pt -> getPermissionSqlTemplate(pt)) - .collect(Collectors.toList()); - - Map roleDTOMap = new HashMap<>(); - permissionsSQLTemplates.forEach(p -> { - Iterable permissionEntities = permissionRepository.findByValueLike(p, PERMISSION_ENTITY_GRAPH); - for (PermissionEntity permissionEntity : permissionEntities) { - Set roles = rolesForEntity.get(permissionEntity.getValue()); - if (roles == null) { - rolesForEntity.put(permissionEntity.getValue(), new HashSet<>()); - } - Set cachedRoles = rolesForEntity.get(permissionEntity.getValue()); - permissionEntity.getRolePermissions().forEach(rp -> { - RoleDTO roleDTO = roleDTOMap.get(rp.getRole().getId()); - if (roleDTO == null) { - roleDTO = conversionService.convert(rp.getRole(), RoleDTO.class); - roleDTOMap.put(roleDTO.getId(), roleDTO); - } - cachedRoles.add(roleDTO); - }); - } - }); - } - } - - public List getRolesHavingPermissions(EntityType entityType, Number id) { - Set permissionTemplates = getTemplatesForType(entityType, AccessType.WRITE).keySet(); - preparePermissionCache(entityType, permissionTemplates); - - List permissions = permissionTemplates.stream() - .map(pt -> getPermission(pt, id)) + List permissions = permissionTemplates.stream() + .map(pt -> new WildcardPermission(getPermission(pt, id))) .collect(Collectors.toList()); - int fitCount = permissions.size(); - Map roleMap = permissions.stream() - .filter(p -> permissionCache.get().get(entityType).get(p) != null) - .flatMap(p -> permissionCache.get().get(entityType).get(p).stream()) - .collect(Collectors.groupingBy(Function.identity(), Collectors.counting())); - List roles = roleMap.entrySet().stream() - .filter(es -> es.getValue() == fitCount) - .map(es -> es.getKey()) - .collect(Collectors.toList()); - return roles; - } - - public List getRolesHavingReadPermissions(EntityType entityType, Number id) { - Set permissionTemplates = getTemplatesForType(entityType, AccessType.READ).keySet(); - preparePermissionCache(entityType, permissionTemplates); - - List permissions = permissionTemplates.stream() - .map(pt -> getPermission(pt, id)) - .collect(Collectors.toList()); - int fitCount = permissions.size(); - Map roleMap = permissions.stream() - .filter(p -> permissionCache.get().get(entityType).get(p) != null) - .flatMap(p -> permissionCache.get().get(entityType).get(p).stream()) - .collect(Collectors.groupingBy(Function.identity(), Collectors.counting())); - List roles = roleMap.entrySet().stream() - .filter(es -> es.getValue() == fitCount) - .map(es -> es.getKey()) - .collect(Collectors.toList()); - return roles; - } - - public void clearPermissionCache() { - this.permissionCache.set(new ConcurrentHashMap<>()); + return permissions; } - public boolean hasWriteAccess(CommonEntity entity) { + public boolean hasAccess(CommonEntity entity, AccessType accessType) { boolean hasAccess = false; if (securityEnabled && entity.getCreatedBy() != null) { try { + Subject subject = SecurityUtils.getSubject(); String login = this.permissionManager.getSubjectName(); UserSimpleAuthorizationInfo authorizationInfo = this.permissionManager.getAuthorizationInfo(login); if (Objects.equals(authorizationInfo.getUserId(), entity.getCreatedBy().getId())) { hasAccess = true; // the role is the one that created the artifact } else { EntityType entityType = entityPermissionSchemaResolver.getEntityType(entity.getClass()); - - List roles = getRolesHavingPermissions(entityType, entity.getId()); - - Collection userRoles = authorizationInfo.getRoles(); - hasAccess = roles.stream() - .anyMatch(r -> userRoles.stream() - .anyMatch(re -> re.equals(r.getName()))); + List permsToCheck = getEntityPermissions(entityType, entity.getId(), accessType); + hasAccess = permsToCheck.stream().allMatch(p -> subject.isPermitted(p)); } } catch (Exception e) { logger.error("Error getting user roles and permissions", e); @@ -277,43 +203,24 @@ public boolean hasWriteAccess(CommonEntity entity) { } return hasAccess; } - + + public boolean hasWriteAccess(CommonEntity entity) { + return hasAccess(entity, AccessType.WRITE); + } public boolean hasReadAccess(CommonEntity entity) { - boolean hasAccess = false; - if (securityEnabled && entity.getCreatedBy() != null) { - try { - String login = this.permissionManager.getSubjectName(); - UserSimpleAuthorizationInfo authorizationInfo = this.permissionManager.getAuthorizationInfo(login); - if (Objects.equals(authorizationInfo.getUserId(), entity.getCreatedBy().getId())){ - hasAccess = true; // the role is the one that created the artifact - } else { - EntityType entityType = entityPermissionSchemaResolver.getEntityType(entity.getClass()); - - List roles = getRolesHavingReadPermissions(entityType, entity.getId()); - - Collection userRoles = authorizationInfo.getRoles(); - hasAccess = roles.stream() - .anyMatch(r -> userRoles.stream() - .anyMatch(re -> re.equals(r.getName()))); - } - } catch (Exception e) { - logger.error("Error getting user roles and permissions", e); - throw new RuntimeException(e); - } - } - return hasAccess; + return hasAccess(entity, AccessType.READ); } public void fillWriteAccess(CommonEntity entity, CommonEntityDTO entityDTO) { if (securityEnabled && entity.getCreatedBy() != null) { - entityDTO.setHasWriteAccess(hasWriteAccess(entity)); + entityDTO.setHasWriteAccess(hasAccess(entity, AccessType.WRITE)); } } public void fillReadAccess(CommonEntity entity, CommonEntityDTO entityDTO) { if (securityEnabled && entity.getCreatedBy() != null) { - entityDTO.setHasReadAccess(hasReadAccess(entity)); + entityDTO.setHasReadAccess(hasAccess(entity, AccessType.READ)); } } diff --git a/src/main/java/org/ohdsi/webapi/security/model/UserSimpleAuthorizationInfo.java b/src/main/java/org/ohdsi/webapi/security/model/UserSimpleAuthorizationInfo.java index e4d09e44a8..5db3519fa4 100644 --- a/src/main/java/org/ohdsi/webapi/security/model/UserSimpleAuthorizationInfo.java +++ b/src/main/java/org/ohdsi/webapi/security/model/UserSimpleAuthorizationInfo.java @@ -1,25 +1,39 @@ package org.ohdsi.webapi.security.model; +import java.util.List; +import java.util.Map; +import org.apache.shiro.authz.Permission; import org.apache.shiro.authz.SimpleAuthorizationInfo; public class UserSimpleAuthorizationInfo extends SimpleAuthorizationInfo { - private Long userId; - private String login; + private Long userId; + private String login; + private Map> permissionIdx; - public Long getUserId() { - return userId; - } + + public Long getUserId() { + return userId; + } - public void setUserId(Long userId) { - this.userId = userId; - } + public void setUserId(Long userId) { + this.userId = userId; + } - public String getLogin() { - return login; - } + public String getLogin() { + return login; + } + + public void setLogin(String login) { + this.login = login; + } + + public Map> getPermissionIdx() { + return permissionIdx; + } + + public void setPermissionIdx(Map> permissionIdx) { + this.permissionIdx = permissionIdx; + } - public void setLogin(String login) { - this.login = login; - } } diff --git a/src/main/java/org/ohdsi/webapi/service/UserService.java b/src/main/java/org/ohdsi/webapi/service/UserService.java index 2c786a8440..5dd56bcc18 100644 --- a/src/main/java/org/ohdsi/webapi/service/UserService.java +++ b/src/main/java/org/ohdsi/webapi/service/UserService.java @@ -51,7 +51,7 @@ public static class User implements Comparable { public String login; public String name; public List permissions; - public JsonNode permissionIdx; + public Map> permissionIdx; public User() {} diff --git a/src/main/java/org/ohdsi/webapi/shiro/PermissionManager.java b/src/main/java/org/ohdsi/webapi/shiro/PermissionManager.java index 96c6d13c55..43ff4ace64 100644 --- a/src/main/java/org/ohdsi/webapi/shiro/PermissionManager.java +++ b/src/main/java/org/ohdsi/webapi/shiro/PermissionManager.java @@ -1,8 +1,6 @@ package org.ohdsi.webapi.shiro; -import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.node.ArrayNode; import com.odysseusinc.logging.event.AddUserEvent; import com.odysseusinc.logging.event.DeleteRoleEvent; import org.apache.shiro.SecurityUtils; @@ -28,13 +26,18 @@ import org.springframework.transaction.annotation.Transactional; import java.security.Principal; +import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.stream.Collectors; import org.apache.commons.lang3.StringUtils; +import org.apache.shiro.authz.Permission; +import org.apache.shiro.authz.permission.WildcardPermission; import org.ohdsi.circe.helper.ResourceHelper; import org.springframework.beans.factory.annotation.Value; import org.springframework.jdbc.core.JdbcTemplate; @@ -71,14 +74,11 @@ public class PermissionManager { @Autowired private JdbcTemplate jdbcTemplate; - @Autowired - private ObjectMapper objectMapper; - private ThreadLocal> authorizationInfoCache = ThreadLocal.withInitial(ConcurrentHashMap::new); public static class PermissionsDTO { - public JsonNode permissions = null; + public Map> permissions = null; } public RoleEntity addRole(String roleName, boolean isSystem) { @@ -132,6 +132,12 @@ public Iterable getRoles(boolean includePersonalRoles) { } } + /** + * Return the UserSimpleAuthorizastionInfo which contains the login, roles and permissions for the specified login + * + * @param login The login to fetch the authorization info + * @return A UserSimpleAuthorizationInfo containing roles and permissions. + */ public UserSimpleAuthorizationInfo getAuthorizationInfo(final String login) { return authorizationInfoCache.get().computeIfAbsent(login, newLogin -> { @@ -148,14 +154,21 @@ public UserSimpleAuthorizationInfo getAuthorizationInfo(final String login) { for (UserRoleEntity userRole: userEntity.getUserRoles()) { info.addRole(userRole.getRole().getName()); } - final Set permissionNames = new LinkedHashSet<>(); - final Set permissions = this.getUserPermissions(userEntity); - for (PermissionEntity permission : permissions) { - permissionNames.add(permission.getValue()); + // convert permission index from queryUserPermissions() into a map of WildcardPermissions + Map> permsIdx = this.queryUserPermissions(newLogin).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; }); } @@ -345,7 +358,7 @@ public PermissionsDTO queryUserPermissions(final String login) { return rs.getString("value"); }); PermissionsDTO permDto = new PermissionsDTO(); - permDto.permissions = permsToJsonNode(permissions); + permDto.permissions = permsToMap(permissions); return permDto; } @@ -354,23 +367,23 @@ public PermissionsDTO queryUserPermissions(final String login) { * the first element of each permission as a key, and the List of * permissions that start with the key as the value */ - private JsonNode permsToJsonNode(List permissions) { + private Map> permsToMap(List permissions) { - Map resultMap = new HashMap<>(); + Map> resultMap = new HashMap<>(); // Process each input string for (String inputString : permissions) { String[] parts = inputString.split(":"); String key = parts[0]; // Create a new JsonArray for the key if it doesn't exist - resultMap.putIfAbsent(key, objectMapper.createArrayNode()); + resultMap.putIfAbsent(key, new ArrayList<>()); // Add the value to the JsonArray resultMap.get(key).add(inputString); } // Convert the resultMap to a JsonNode - return objectMapper.valueToTree(resultMap); + return resultMap; } private Set getRolePermissions(RoleEntity role) { @@ -474,7 +487,7 @@ private RolePermissionEntity addPermission(final RoleEntity role, final Permissi } private boolean isRelationAllowed(final String relationStatus) { - return relationStatus == null || relationStatus == RequestStatus.APPROVED; + return relationStatus == null || relationStatus.equals(RequestStatus.APPROVED); } private UserRoleEntity addUser(final UserEntity user, final RoleEntity role, diff --git a/src/main/java/org/ohdsi/webapi/shiro/filters/CacheFilter.java b/src/main/java/org/ohdsi/webapi/shiro/filters/CacheFilter.java index ce12edbe45..d76aa8a921 100644 --- a/src/main/java/org/ohdsi/webapi/shiro/filters/CacheFilter.java +++ b/src/main/java/org/ohdsi/webapi/shiro/filters/CacheFilter.java @@ -19,9 +19,6 @@ public class CacheFilter implements Filter { @Autowired private PermissionManager permissionManager; - @Autowired - private PermissionService permissionService; - @Override public void init(FilterConfig filterConfig) throws ServletException { @@ -31,7 +28,6 @@ public void init(FilterConfig filterConfig) throws ServletException { public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { permissionManager.clearAuthorizationInfoCache(); - permissionService.clearPermissionCache(); chain.doFilter(request, response); } diff --git a/src/main/java/org/ohdsi/webapi/shiro/realms/JwtAuthRealm.java b/src/main/java/org/ohdsi/webapi/shiro/realms/JwtAuthRealm.java index 2c1a4514db..e7245b1b49 100644 --- a/src/main/java/org/ohdsi/webapi/shiro/realms/JwtAuthRealm.java +++ b/src/main/java/org/ohdsi/webapi/shiro/realms/JwtAuthRealm.java @@ -1,12 +1,17 @@ package org.ohdsi.webapi.shiro.realms; +import java.util.ArrayList; +import java.util.List; +import org.apache.commons.lang3.StringUtils; import org.apache.shiro.authc.AuthenticationException; import org.apache.shiro.authc.AuthenticationInfo; import org.apache.shiro.authc.AuthenticationToken; import org.apache.shiro.authc.SimpleAuthenticationInfo; import org.apache.shiro.authz.AuthorizationInfo; +import org.apache.shiro.authz.Permission; import org.apache.shiro.realm.AuthorizingRealm; import org.apache.shiro.subject.PrincipalCollection; +import org.ohdsi.webapi.security.model.UserSimpleAuthorizationInfo; import org.ohdsi.webapi.shiro.PermissionManager; import org.ohdsi.webapi.shiro.tokens.JwtAuthToken; @@ -37,5 +42,20 @@ protected AuthorizationInfo doGetAuthorizationInfo(PrincipalCollection principal @Override protected AuthenticationInfo doGetAuthenticationInfo(AuthenticationToken at) throws AuthenticationException { return new SimpleAuthenticationInfo(at.getPrincipal(), "", getName()); - } + } + + @Override + protected boolean isPermitted(Permission permission, AuthorizationInfo info) { + // for speed, we check the permission against the set of permissions found by the key '*' and the first element of the permission. + String permPrefix = StringUtils.split(permission.toString(), ":")[0]; + // we only need to check * perms and perms that begin with the same prefix (up to first :) as the requested permission + // starting with perms that start with * + List permsToCheck = new ArrayList(((UserSimpleAuthorizationInfo)info).getPermissionIdx().getOrDefault("*", new ArrayList<>())); + // adding those permissiosn that start with the permPrefix + permsToCheck.addAll(((UserSimpleAuthorizationInfo)info).getPermissionIdx().getOrDefault(permPrefix, new ArrayList<>())); + + // do check + return permsToCheck.stream().anyMatch(permToCheck -> permToCheck.implies(permission)); + + } } From 65589824761ec435e25496de8a0dd3a0adbc82b4 Mon Sep 17 00:00:00 2001 From: Chris Knoll Date: Tue, 16 Apr 2024 04:34:20 -0400 Subject: [PATCH 2/4] Added test cases. Reordered test scoped dependencies in pom.xml. Refactored shared methods to AbstractDatabaseTest. --- pom.xml | 81 ++++++----- .../ohdsi/webapi/AbstractDatabaseTest.java | 129 ++++++++++++------ .../webapi/pathway/PathwayAnalysisTest.java | 25 ---- .../ohdsi/webapi/security/PermissionTest.java | 88 ++++++++++++ .../resources/permission/permsTest_PREP.json | 65 +++++++++ 5 files changed, 277 insertions(+), 111 deletions(-) create mode 100644 src/test/java/org/ohdsi/webapi/security/PermissionTest.java create mode 100644 src/test/resources/permission/permsTest_PREP.json diff --git a/pom.xml b/pom.xml index f799a72a71..c3778b5502 100644 --- a/pom.xml +++ b/pom.xml @@ -761,18 +761,6 @@ xml-security-impl 1.0 - - org.springframework.boot - spring-boot-starter-test - ${spring.boot.version} - test - - - com.vaadin.external.google - android-json - - - @@ -1029,24 +1017,6 @@ jasypt-hibernate4 1.9.2 - - org.dbunit - dbunit - 2.7.0 - test - - - com.github.springtestdbunit - spring-test-dbunit - 1.3.0 - test - - - pl.pragmatists - JUnitParams - 1.1.0 - test - org.bouncycastle bcprov-jdk15on @@ -1208,6 +1178,16 @@ commonmark-ext-gfm-tables 0.15.2 + + org.dom4j + dom4j + ${dom4j.version} + + + com.cloudbees + syslog-java-client + 1.1.7 + com.opentable.components otj-pg-embedded @@ -1215,24 +1195,43 @@ test - com.github.mjeanroy - dbunit-plus - 2.0.1 - test + org.springframework.boot + spring-boot-starter-test + ${spring.boot.version} + test + + + com.vaadin.external.google + android-json + + - org.dom4j - dom4j - ${dom4j.version} + org.dbunit + dbunit + 2.7.0 + test - com.cloudbees - syslog-java-client - 1.1.7 + com.github.springtestdbunit + spring-test-dbunit + 1.3.0 + test + + + pl.pragmatists + JUnitParams + 1.1.0 + test + + + com.github.mjeanroy + dbunit-plus + 2.0.1 + test - webapi-oracle diff --git a/src/test/java/org/ohdsi/webapi/AbstractDatabaseTest.java b/src/test/java/org/ohdsi/webapi/AbstractDatabaseTest.java index afb076d75b..9b7e5417d5 100644 --- a/src/test/java/org/ohdsi/webapi/AbstractDatabaseTest.java +++ b/src/test/java/org/ohdsi/webapi/AbstractDatabaseTest.java @@ -1,5 +1,6 @@ package org.ohdsi.webapi; +import com.github.mjeanroy.dbunit.core.dataset.DataSetFactory; import org.junit.ClassRule; import org.junit.Ignore; import org.junit.rules.ExternalResource; @@ -16,63 +17,101 @@ import java.sql.Driver; import java.sql.DriverManager; import java.sql.SQLException; +import org.dbunit.DatabaseUnitException; +import org.dbunit.database.DatabaseConfig; +import org.dbunit.database.DatabaseDataSourceConnection; +import org.dbunit.database.IDatabaseConnection; +import org.dbunit.dataset.IDataSet; +import org.dbunit.ext.postgresql.PostgresqlDataTypeFactory; +import org.dbunit.operation.DatabaseOperation; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.fail; @SpringBootTest @RunWith(SpringRunner.class) @TestPropertySource(locations = "/application-test.properties") public abstract class AbstractDatabaseTest { - static class JdbcTemplateTestWrapper extends ExternalResource { - @Override - protected void before() throws Throwable { - jdbcTemplate = new JdbcTemplate(getDataSource()); - try { - // note for future reference: should probably either define a TestContext DataSource with these params - // or make it so this proparty is only set once (during database initialization) since the below will run for each test class (but only be effective once) - System.setProperty("datasource.url", getDataSource().getConnection().getMetaData().getURL()); - System.setProperty("flyway.datasource.url", System.getProperty("datasource.url")); - } catch (Exception ex) { - throw new RuntimeException(ex); - } - } + + static class JdbcTemplateTestWrapper extends ExternalResource { + + @Override + protected void before() throws Throwable { + jdbcTemplate = new JdbcTemplate(getDataSource()); + try { + // note for future reference: should probably either define a TestContext DataSource with these params + // or make it so this proparty is only set once (during database initialization) since the below will run for each test class (but only be effective once) + System.setProperty("datasource.url", getDataSource().getConnection().getMetaData().getURL()); + System.setProperty("flyway.datasource.url", System.getProperty("datasource.url")); + } catch (Exception ex) { + throw new RuntimeException(ex); + } } + } - static class DriverExcludeTestWrapper extends ExternalResource { - @Override - protected void before() throws Throwable { - // Put the redshift driver at the end so that it doesn't - // conflict with postgres queries - java.util.Enumeration drivers = DriverManager.getDrivers(); - while (drivers.hasMoreElements()) { - Driver driver = drivers.nextElement(); - if (driver.getClass().getName().contains("com.amazon.redshift.jdbc")) { - try { - DriverManager.deregisterDriver(driver); - DriverManager.registerDriver(driver); - } catch (SQLException e) { - throw new RuntimeException("Could not deregister redshift driver", e); - } - } - } + static class DriverExcludeTestWrapper extends ExternalResource { + + @Override + protected void before() throws Throwable { + // Put the redshift driver at the end so that it doesn't + // conflict with postgres queries + java.util.Enumeration drivers = DriverManager.getDrivers(); + while (drivers.hasMoreElements()) { + Driver driver = drivers.nextElement(); + if (driver.getClass().getName().contains("com.amazon.redshift.jdbc")) { + try { + DriverManager.deregisterDriver(driver); + DriverManager.registerDriver(driver); + } catch (SQLException e) { + throw new RuntimeException("Could not deregister redshift driver", e); + } } + } } + } - @ClassRule - public static TestRule chain = RuleChain.outerRule(new DriverExcludeTestWrapper()) - .around(pg = new PostgresSingletonRule()) - .around(new JdbcTemplateTestWrapper()); + @ClassRule + public static TestRule chain = RuleChain.outerRule(new DriverExcludeTestWrapper()) + .around(pg = new PostgresSingletonRule()) + .around(new JdbcTemplateTestWrapper()); - protected static PostgresSingletonRule pg; + protected static PostgresSingletonRule pg; - protected static JdbcTemplate jdbcTemplate; + protected static JdbcTemplate jdbcTemplate; - protected static DataSource getDataSource() { - return pg.getEmbeddedPostgres().getPostgresDatabase(); - } - - protected void truncateTable (String tableName) { - jdbcTemplate.execute(String.format("TRUNCATE %s CASCADE",tableName)); - } - protected void resetSequence(String sequenceName) { - jdbcTemplate.execute(String.format("ALTER SEQUENCE %s RESTART WITH 1", sequenceName)); + protected static DataSource getDataSource() { + return pg.getEmbeddedPostgres().getPostgresDatabase(); + } + + protected void truncateTable(String tableName) { + jdbcTemplate.execute(String.format("TRUNCATE %s CASCADE", tableName)); + } + + protected void resetSequence(String sequenceName) { + jdbcTemplate.execute(String.format("ALTER SEQUENCE %s RESTART WITH 1", sequenceName)); + } + + protected static IDatabaseConnection getConnection() throws SQLException { + final IDatabaseConnection con = new DatabaseDataSourceConnection(getDataSource()); + con.getConfig().setProperty(DatabaseConfig.FEATURE_QUALIFIED_TABLE_NAMES, true); + con.getConfig().setProperty(DatabaseConfig.PROPERTY_DATATYPE_FACTORY, new PostgresqlDataTypeFactory()); + return con; + } + + protected void loadPrepData(String[] datasetPaths) throws Exception { + loadPrepData(datasetPaths, DatabaseOperation.CLEAN_INSERT); + } + protected void loadPrepData(String[] datasetPaths, DatabaseOperation dbOp) throws Exception { + final IDatabaseConnection dbUnitCon = getConnection(); + final IDataSet ds = DataSetFactory.createDataSet(datasetPaths); + + assertNotNull("No dataset found", ds); + + try { + dbOp.execute(dbUnitCon, ds); // clean load of the DB. Careful, clean means "delete the old stuff" + } catch (final DatabaseUnitException e) { + fail(e.getMessage()); + } finally { + dbUnitCon.close(); } + } } diff --git a/src/test/java/org/ohdsi/webapi/pathway/PathwayAnalysisTest.java b/src/test/java/org/ohdsi/webapi/pathway/PathwayAnalysisTest.java index 0c4aa72881..9ff81cecb8 100644 --- a/src/test/java/org/ohdsi/webapi/pathway/PathwayAnalysisTest.java +++ b/src/test/java/org/ohdsi/webapi/pathway/PathwayAnalysisTest.java @@ -24,16 +24,12 @@ import java.util.Collection; import org.dbunit.Assertion; import org.dbunit.DatabaseUnitException; -import org.dbunit.database.DatabaseConfig; -import org.dbunit.database.DatabaseDataSourceConnection; import org.dbunit.database.IDatabaseConnection; import org.dbunit.dataset.CompositeDataSet; import org.dbunit.dataset.IDataSet; import org.dbunit.dataset.ITable; -import org.dbunit.ext.postgresql.PostgresqlDataTypeFactory; import org.dbunit.operation.DatabaseOperation; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.junit.Assert.assertEquals; @@ -96,12 +92,6 @@ public class PathwayAnalysisTest extends AbstractDatabaseTest { private static SerializedPathwayAnalysisToPathwayAnalysisConverter converter = new SerializedPathwayAnalysisToPathwayAnalysisConverter(); - private static IDatabaseConnection getConnection() throws SQLException { - final IDatabaseConnection con = new DatabaseDataSourceConnection(getDataSource()); - con.getConfig().setProperty(DatabaseConfig.FEATURE_QUALIFIED_TABLE_NAMES, true); - con.getConfig().setProperty(DatabaseConfig.PROPERTY_DATATYPE_FACTORY, new PostgresqlDataTypeFactory()); - return con; - } @Before public void setUp() throws Exception { @@ -183,21 +173,6 @@ private Source getCdmSource() throws SQLException { return source; } - private void loadPrepData(String[] datasetPaths) throws Exception { - final IDatabaseConnection dbUnitCon = getConnection(); - final IDataSet ds = DataSetFactory.createDataSet(datasetPaths); - - assertNotNull("No dataset found", ds); - - try { - DatabaseOperation.CLEAN_INSERT.execute(dbUnitCon, ds); // clean load of the DB. Careful, clean means "delete the old stuff" - } catch (final DatabaseUnitException e) { - fail(e.getMessage()); - } finally { - dbUnitCon.close(); - } - } - private void generateAnalysis(PathwayAnalysisEntity entity) throws Exception { JobExecutionResource executionResource = pathwayService.generatePathways(entity.getId(), SOURCE_ID); PathwayAnalysisGenerationEntity generationEntity; diff --git a/src/test/java/org/ohdsi/webapi/security/PermissionTest.java b/src/test/java/org/ohdsi/webapi/security/PermissionTest.java new file mode 100644 index 0000000000..07431e0999 --- /dev/null +++ b/src/test/java/org/ohdsi/webapi/security/PermissionTest.java @@ -0,0 +1,88 @@ +/* + * Copyright 2024 cknoll1. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.ohdsi.webapi.security; + +import java.util.Arrays; +import org.apache.shiro.SecurityUtils; +import org.apache.shiro.subject.SimplePrincipalCollection; +import org.apache.shiro.subject.Subject; +import org.apache.shiro.util.ThreadContext; +import org.apache.shiro.web.mgt.DefaultWebSecurityManager; +import org.dbunit.operation.DatabaseOperation; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import org.junit.Before; +import org.junit.Test; +import org.ohdsi.webapi.AbstractDatabaseTest; +import org.ohdsi.webapi.shiro.PermissionManager; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.test.context.TestPropertySource; + +/** + * + * @author cknoll1 + */ +@TestPropertySource(properties = { + "security.provider=AtlasRegularSecurity" +}) +public class PermissionTest extends AbstractDatabaseTest { + + @Autowired + private PermissionManager permissionManager; + + @Value("${security.provider}") + String securityProvider; + + @Autowired + private DefaultWebSecurityManager securityManager; + + private Subject subject; + + @Before + public void setup() { + // Set the SecurityManager for the current thread + SimplePrincipalCollection principalCollection = new SimplePrincipalCollection(); + principalCollection.addAll(Arrays.asList("permsTest"),"testRealm"); + subject = new Subject.Builder(securityManager) + .authenticated(true) + .principals(principalCollection) + .buildSubject(); + ThreadContext.bind(subject); + } + + @Test + public void permsTest() throws Exception { + // Set up the mock Subject as the currently authenticated user + Subject s = SecurityUtils.getSubject(); + String subjetName = permissionManager.getSubjectName(); + + final String[] testDataSetsPaths = new String[] {"/permission/permsTest_PREP.json" }; + + loadPrepData(testDataSetsPaths); + + // subject can manage printer1 and printer2, can do print and query on any printer. + assertTrue(s.isPermitted("printer:manage:printer1")); + assertTrue(s.isPermitted("printer:manage:printer2")); + assertFalse(s.isPermitted("printer:manage:printer3")); + assertTrue(s.isPermitted("printer:query:printer4")); + assertTrue(s.isPermitted("printer:print:printer5")); + + loadPrepData(testDataSetsPaths, DatabaseOperation.DELETE); + + } + +} diff --git a/src/test/resources/permission/permsTest_PREP.json b/src/test/resources/permission/permsTest_PREP.json new file mode 100644 index 0000000000..8ad261396e --- /dev/null +++ b/src/test/resources/permission/permsTest_PREP.json @@ -0,0 +1,65 @@ +{ + "public.sec_user": [ + { + "id":1001, + "login":"permsTest", + "name":"Perms Test User", + "origin":"SYSTEM" + } + ], + "public.sec_role": [ + { + "id": 1001, + "name":"permsTest", + "system_role":false + } + ], + "public.sec_permission" : [ + { + "id": 1001, + "value":"printer:manage:printer1" + }, + { + "id": 1002, + "value":"printer:manage:printer2" + }, + { + "id": 1003, + "value":"printer:query:*" + }, + { + "id": 1004, + "value":"printer:print" + } + ], + "public.sec_role_permission" : [ + { + "id":1001, + "role_id":1001, + "permission_id":1001 + }, + { + "id":1002, + "role_id":1001, + "permission_id":1002 + }, + { + "id":1003, + "role_id":1001, + "permission_id":1003 + }, + { + "id":1004, + "role_id":1001, + "permission_id":1004 + } + ], + "public.sec_user_role": [ + { + "id": 1001, + "user_id":1001, + "role_id":1001, + "origin":"SYSTEM" + } + ] +} From 9326c9707ce8112b8c9b5c876b0a7e8c87704d2d Mon Sep 17 00:00:00 2001 From: Chris Knoll Date: Tue, 16 Apr 2024 10:30:43 -0400 Subject: [PATCH 3/4] Change DB inserts to use REFRESH. Added test to grant * permission to user. --- .../ohdsi/webapi/security/PermissionTest.java | 20 +++++++++- .../permission/wildcardTest_PREP.json | 38 +++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) create mode 100644 src/test/resources/permission/wildcardTest_PREP.json diff --git a/src/test/java/org/ohdsi/webapi/security/PermissionTest.java b/src/test/java/org/ohdsi/webapi/security/PermissionTest.java index 07431e0999..5064ea75b8 100644 --- a/src/test/java/org/ohdsi/webapi/security/PermissionTest.java +++ b/src/test/java/org/ohdsi/webapi/security/PermissionTest.java @@ -66,13 +66,12 @@ public void setup() { @Test public void permsTest() throws Exception { - // Set up the mock Subject as the currently authenticated user Subject s = SecurityUtils.getSubject(); String subjetName = permissionManager.getSubjectName(); final String[] testDataSetsPaths = new String[] {"/permission/permsTest_PREP.json" }; - loadPrepData(testDataSetsPaths); + loadPrepData(testDataSetsPaths, DatabaseOperation.REFRESH); // subject can manage printer1 and printer2, can do print and query on any printer. assertTrue(s.isPermitted("printer:manage:printer1")); @@ -85,4 +84,21 @@ public void permsTest() throws Exception { } + @Test + public void wildcardTest() throws Exception { + Subject s = SecurityUtils.getSubject(); + String subjetName = permissionManager.getSubjectName(); + + final String[] testDataSetsPaths = new String[] {"/permission/wildcardTest_PREP.json" }; + + loadPrepData(testDataSetsPaths, DatabaseOperation.REFRESH); + + // subject has * permisison, so any permisison test is true + assertTrue(s.isPermitted("printer:manage:printer1")); + assertTrue(s.isPermitted("printer")); + + loadPrepData(testDataSetsPaths, DatabaseOperation.DELETE); + + } + } diff --git a/src/test/resources/permission/wildcardTest_PREP.json b/src/test/resources/permission/wildcardTest_PREP.json new file mode 100644 index 0000000000..6f08990e8f --- /dev/null +++ b/src/test/resources/permission/wildcardTest_PREP.json @@ -0,0 +1,38 @@ +{ + "public.sec_user": [ + { + "id":1001, + "login":"permsTest", + "name":"Perms Test User", + "origin":"SYSTEM" + } + ], + "public.sec_role": [ + { + "id": 1001, + "name":"permsTest", + "system_role":false + } + ], + "public.sec_permission" : [ + { + "id": 1001, + "value":"*" + } + ], + "public.sec_role_permission" : [ + { + "id":1001, + "role_id":1001, + "permission_id":1001 + } + ], + "public.sec_user_role": [ + { + "id": 1001, + "user_id":1001, + "role_id":1001, + "origin":"SYSTEM" + } + ] +} From d6e9cbc0c02c87f6153fb54b31456ef2ba940b9f Mon Sep 17 00:00:00 2001 From: Chris Knoll Date: Tue, 16 Apr 2024 23:17:06 -0400 Subject: [PATCH 4/4] Clear security cache between tests. --- .../java/org/ohdsi/webapi/security/PermissionTest.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/test/java/org/ohdsi/webapi/security/PermissionTest.java b/src/test/java/org/ohdsi/webapi/security/PermissionTest.java index 5064ea75b8..c8a83be095 100644 --- a/src/test/java/org/ohdsi/webapi/security/PermissionTest.java +++ b/src/test/java/org/ohdsi/webapi/security/PermissionTest.java @@ -66,6 +66,8 @@ public void setup() { @Test public void permsTest() throws Exception { + // need to clear authorization cache before each test + permissionManager.clearAuthorizationInfoCache(); Subject s = SecurityUtils.getSubject(); String subjetName = permissionManager.getSubjectName(); @@ -86,11 +88,10 @@ public void permsTest() throws Exception { @Test public void wildcardTest() throws Exception { + // need to clear authorization cache before each test + permissionManager.clearAuthorizationInfoCache(); Subject s = SecurityUtils.getSubject(); - String subjetName = permissionManager.getSubjectName(); - final String[] testDataSetsPaths = new String[] {"/permission/wildcardTest_PREP.json" }; - loadPrepData(testDataSetsPaths, DatabaseOperation.REFRESH); // subject has * permisison, so any permisison test is true