Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Permission Checks now use Wildcard semantics. #2355

Merged
merged 6 commits into from
Apr 23, 2024
Merged

Permission Checks now use Wildcard semantics. #2355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
379d73b
Removed role cache from Permission Service.
chrisknoll Feb 27, 2024
4f8583f
Merge remote-tracking branch 'remotes/origin/master' into issue-2353-…
chrisknoll Apr 9, 2024
3ae4730
Merge remote-tracking branch 'remotes/origin/master' into issue-2353-…
chrisknoll Apr 15, 2024
6558982
Added test cases.
chrisknoll Apr 16, 2024
9326c97
Change DB inserts to use REFRESH.
chrisknoll Apr 16, 2024
d6e9cbc
Clear security cache between tests.
chrisknoll Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 40 additions & 41 deletions pom.xml
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading