Skip to content

Commit

Permalink
Permission Checks now use Wildcard semantics. (OHDSI#2355)
Browse files Browse the repository at this point in the history 
Permission checks are using Subject.isPermitted() which honors wildcard semantics.
Changed read/write permission checks to use permissions instead of roles.
Removed role cache from Permission Service.
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 OHDSI#2353.

* Added test cases.
Reordered test scoped dependencies in pom.xml.
Refactored shared methods to AbstractDatabaseTest.
  • Loading branch information
@chrisknoll
chrisknoll authored Apr 23, 2024
1 parent f347d2d commit 6a43da5
Show file tree
Hide file tree
Showing 12 changed files with 439 additions and 268 deletions.
81 changes: 40 additions & 41 deletions pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -761,18 +761,6 @@
<artifactId>xml-security-impl</artifactId>
<version>1.0</version>
</dependency>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-test</artifactId>
<version>${spring.boot.version}</version>
<scope>test</scope>
<exclusions>
<exclusion>
<groupId>com.vaadin.external.google</groupId>
<artifactId>android-json</artifactId>
</exclusion>
</exclusions>
</dependency>
<!-- It is overriding a transitive dependency from the dependency above -->
<!-- After migrating to Spring Boot 2.x we should get rid of it -->
<!-- It is currently inefficient as com.nimbusds:nimbus-jose-jwt has hard bounds [1.3.1,2.3] -->
Expand Down Expand Up @@ -1029,24 +1017,6 @@
<artifactId>jasypt-hibernate4</artifactId>
<version>1.9.2</version>
</dependency>
<dependency>
<groupId>org.dbunit</groupId>
<artifactId>dbunit</artifactId>
<version>2.7.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.github.springtestdbunit</groupId>
<artifactId>spring-test-dbunit</artifactId>
<version>1.3.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>pl.pragmatists</groupId>
<artifactId>JUnitParams</artifactId>
<version>1.1.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcprov-jdk15on</artifactId>
Expand Down Expand Up @@ -1208,31 +1178,60 @@
<artifactId>commonmark-ext-gfm-tables</artifactId>
<version>0.15.2</version>
</dependency>
<dependency>
<groupId>org.dom4j</groupId>
<artifactId>dom4j</artifactId>
<version>${dom4j.version}</version>
</dependency>
<dependency>
<groupId>com.cloudbees</groupId>
<artifactId>syslog-java-client</artifactId>
<version>1.1.7</version>
</dependency>
<dependency>
<groupId>com.opentable.components</groupId>
<artifactId>otj-pg-embedded</artifactId>
<version>0.13.1</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.github.mjeanroy</groupId>
<artifactId>dbunit-plus</artifactId>
<version>2.0.1</version>
<scope>test</scope>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-test</artifactId>
<version>${spring.boot.version}</version>
<scope>test</scope>
<exclusions>
<exclusion>
<groupId>com.vaadin.external.google</groupId>
<artifactId>android-json</artifactId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.dom4j</groupId>
<artifactId>dom4j</artifactId>
<version>${dom4j.version}</version>
<groupId>org.dbunit</groupId>
<artifactId>dbunit</artifactId>
<version>2.7.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.cloudbees</groupId>
<artifactId>syslog-java-client</artifactId>
<version>1.1.7</version>
<groupId>com.github.springtestdbunit</groupId>
<artifactId>spring-test-dbunit</artifactId>
<version>1.3.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>pl.pragmatists</groupId>
<artifactId>JUnitParams</artifactId>
<version>1.1.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.github.mjeanroy</groupId>
<artifactId>dbunit-plus</artifactId>
<version>2.0.1</version>
<scope>test</scope>
</dependency>
</dependencies>
<profiles>

<profile>
<id>webapi-oracle</id>
<properties>
Expand Down
149 changes: 28 additions & 121 deletions src/main/java/org/ohdsi/webapi/security/PermissionService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand All @@ -64,9 +62,6 @@ public class PermissionService {
@Value("#{!'${security.provider}'.equals('DisabledSecurity')}")
private boolean securityEnabled;

private ThreadLocal<ConcurrentHashMap<EntityType, ConcurrentHashMap<String, Set<RoleDTO>>>> permissionCache =
ThreadLocal.withInitial(ConcurrentHashMap::new);

private final EntityGraph PERMISSION_ENTITY_GRAPH = EntityGraphUtils.fromAttributePaths("rolePermissions", "rolePermissions.role");

public PermissionService(
Expand Down Expand Up @@ -133,14 +128,14 @@ public void checkCommonEntityOwnership(EntityType entityType, Integer entityId)

public Map<String, String> 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<RoleEntity> finaAllRolesHavingPermissions(List<String> permissions) {
Expand Down Expand Up @@ -178,97 +173,28 @@ private boolean isCurrentUserOwnerOf(CommonEntity entity) {
return Objects.equals(owner.getLogin(), loggedInUsername);
}

public List<Permission> getEntityPermissions(EntityType entityType, Number id, AccessType accessType) {
Set<String> permissionTemplates = getTemplatesForType(entityType, accessType).keySet();

public void preparePermissionCache(EntityType entityType, Set<String> permissionTemplates) {
if (permissionCache.get().get(entityType) == null) {
final ConcurrentHashMap<String, Set<RoleDTO>> rolesForEntity = new ConcurrentHashMap<>();
permissionCache.get().put(entityType, rolesForEntity);

List<String> permissionsSQLTemplates = permissionTemplates.stream()
.map(pt -> getPermissionSqlTemplate(pt))
.collect(Collectors.toList());

Map<Long, RoleDTO> roleDTOMap = new HashMap<>();
permissionsSQLTemplates.forEach(p -> {
Iterable<PermissionEntity> permissionEntities = permissionRepository.findByValueLike(p, PERMISSION_ENTITY_GRAPH);
for (PermissionEntity permissionEntity : permissionEntities) {
Set<RoleDTO> roles = rolesForEntity.get(permissionEntity.getValue());
if (roles == null) {
rolesForEntity.put(permissionEntity.getValue(), new HashSet<>());
}
Set<RoleDTO> 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<RoleDTO> getRolesHavingPermissions(EntityType entityType, Number id) {
Set<String> permissionTemplates = getTemplatesForType(entityType, AccessType.WRITE).keySet();
preparePermissionCache(entityType, permissionTemplates);

List<String> permissions = permissionTemplates.stream()
.map(pt -> getPermission(pt, id))
List<Permission> permissions = permissionTemplates.stream()
.map(pt -> new WildcardPermission(getPermission(pt, id)))
.collect(Collectors.toList());
int fitCount = permissions.size();
Map<RoleDTO, Long> 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<RoleDTO> roles = roleMap.entrySet().stream()
.filter(es -> es.getValue() == fitCount)
.map(es -> es.getKey())
.collect(Collectors.toList());
return roles;
}

public List<RoleDTO> getRolesHavingReadPermissions(EntityType entityType, Number id) {
Set<String> permissionTemplates = getTemplatesForType(entityType, AccessType.READ).keySet();
preparePermissionCache(entityType, permissionTemplates);

List<String> permissions = permissionTemplates.stream()
.map(pt -> getPermission(pt, id))
.collect(Collectors.toList());
int fitCount = permissions.size();
Map<RoleDTO, Long> 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<RoleDTO> 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<RoleDTO> roles = getRolesHavingPermissions(entityType, entity.getId());

Collection<String> userRoles = authorizationInfo.getRoles();
hasAccess = roles.stream()
.anyMatch(r -> userRoles.stream()
.anyMatch(re -> re.equals(r.getName())));
List<Permission> 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);
Expand All @@ -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<RoleDTO> roles = getRolesHavingReadPermissions(entityType, entity.getId());

Collection<String> 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));
}
}

Expand Down
42 changes: 28 additions & 14 deletions src/main/java/org/ohdsi/webapi/security/model/UserSimpleAuthorizationInfo.java
View file
Original file line number Diff line number Diff line change
@@ -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<String,List<Permission>> 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<String, List<Permission>> getPermissionIdx() {
return permissionIdx;
}

public void setPermissionIdx(Map<String, List<Permission>> permissionIdx) {
this.permissionIdx = permissionIdx;
}

public void setLogin(String login) {
this.login = login;
}
}
2 changes: 1 addition & 1 deletion src/main/java/org/ohdsi/webapi/service/UserService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public static class User implements Comparable<User> {
public String login;
public String name;
public List<Permission> permissions;
public JsonNode permissionIdx;
public Map<String, List<String>> permissionIdx;

public User() {}

Expand Down
Loading

0 comments on commit 6a43da5

Please sign in to comment.