From f05f8724d606642758d6bd43e4e98dfd676702a0 Mon Sep 17 00:00:00 2001 From: pieterlukasse Date: Fri, 7 Jun 2024 18:50:02 +0200 Subject: [PATCH] fix: stricter check in checkCommonEntityOwnership for teamProject ownership check --- .../webapi/security/PermissionController.java | 7 +++++- .../webapi/security/PermissionService.java | 24 ++++++++++++++++--- .../webapi/shiro/Entities/RoleEntity.java | 13 ++++++++++ 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/ohdsi/webapi/security/PermissionController.java b/src/main/java/org/ohdsi/webapi/security/PermissionController.java index 625f5a3f6..e3fc428fb 100644 --- a/src/main/java/org/ohdsi/webapi/security/PermissionController.java +++ b/src/main/java/org/ohdsi/webapi/security/PermissionController.java @@ -6,6 +6,8 @@ import org.ohdsi.webapi.service.UserService; import org.ohdsi.webapi.shiro.Entities.PermissionEntity; import org.ohdsi.webapi.shiro.Entities.RoleEntity; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.ohdsi.webapi.shiro.PermissionManager; import org.springframework.core.convert.ConversionService; import org.springframework.stereotype.Controller; @@ -39,6 +41,7 @@ @Path(value = "/permission") @Transactional public class PermissionController { + private final Logger logger = LoggerFactory.getLogger(PermissionController.class); private final PermissionService permissionService; private final PermissionManager permissionManager; @@ -163,11 +166,13 @@ public void grantEntityPermissionsForRole( // check if user, or his "teamProject" own the entity being given access: permissionService.checkCommonEntityOwnership(entityType, entityId); - // furthermore, check if the entity is being shared with "public" role (roleId==1). If yes, then + // furthermore, check if the entity is being shared to the "public" role (roleId==1). If yes, then // check if user has the necessary global/public sharing permission ("artifact:global:share:put") to do so: if (roleId == RoleEntity.PUBLIC_ROLE_ID) { Subject subject = SecurityUtils.getSubject(); if (!subject.isPermitted(new WildcardPermission("artifact:global:share:put"))) { + logger.error("Permission denied: user {} has no permission for sharing entities globally (making them visible to all with a 'public' role)", + this.permissionManager.getSubjectName()); throw new UnauthorizedException(); } } diff --git a/src/main/java/org/ohdsi/webapi/security/PermissionService.java b/src/main/java/org/ohdsi/webapi/security/PermissionService.java index d1a9df54e..7c00d5e99 100644 --- a/src/main/java/org/ohdsi/webapi/security/PermissionService.java +++ b/src/main/java/org/ohdsi/webapi/security/PermissionService.java @@ -125,10 +125,13 @@ public void checkCommonEntityOwnership(EntityType entityType, Integer entityId) CommonEntity entity = (CommonEntity) entityRepository.getOne((Serializable) conversionService.convert(entityId, idClazz)); if (this.authorizationMode.equals("teamproject")) { - // in teamproject mode, it is sufficient if the user has write permission to this entity, + // in teamproject mode, it is sufficient if the team has ALL write permission to this entity, // as entity ownership and maintenance is a shared responsibility within a team: - boolean hasAccess = hasWriteAccess(entity); - if (!hasAccess) { + RoleEntity teamProjectRole = this.permissionManager.getCurrentTeamProjectRoleForCurrentUser(); + boolean teamHasWriteAccess = roleHasAccess(teamProjectRole, entity, AccessType.WRITE); + if (!teamHasWriteAccess) { + logger.error("Permission denied: teamProject {} does not have write access to the entity {}", + teamProjectRole.getName(), entity.getId()); throw new UnauthorizedException(); } } else { @@ -217,6 +220,21 @@ public boolean hasAccess(CommonEntity entity, AccessType accessType) { } return hasAccess; } + + public boolean roleHasAccess(RoleEntity role, CommonEntity entity, AccessType accessType) { + boolean hasAccess = false; + if (securityEnabled) { + try { + EntityType entityType = entityPermissionSchemaResolver.getEntityType(entity.getClass()); + List permsToCheck = getEntityPermissions(entityType, entity.getId(), accessType); + hasAccess = permsToCheck.stream().allMatch(p -> role.isPermitted(p)); + } catch (Exception e) { + logger.error("Error getting and verifying role's permissions", e); + throw new RuntimeException(e); + } + } + return hasAccess; + } public boolean hasWriteAccess(CommonEntity entity) { return hasAccess(entity, AccessType.WRITE); diff --git a/src/main/java/org/ohdsi/webapi/shiro/Entities/RoleEntity.java b/src/main/java/org/ohdsi/webapi/shiro/Entities/RoleEntity.java index 06d05b808..713cd77cb 100644 --- a/src/main/java/org/ohdsi/webapi/shiro/Entities/RoleEntity.java +++ b/src/main/java/org/ohdsi/webapi/shiro/Entities/RoleEntity.java @@ -11,6 +11,8 @@ import javax.persistence.Id; import javax.persistence.OneToMany; import javax.persistence.Table; +import org.apache.shiro.authz.permission.WildcardPermission; +import org.apache.shiro.authz.Permission; import org.hibernate.annotations.GenericGenerator; import org.hibernate.annotations.Parameter; @@ -90,4 +92,15 @@ public Boolean isSystemRole() { public void setSystemRole(Boolean system) { systemRole = system; } + + public boolean isPermitted(Permission p) { + for (RolePermissionEntity rolePermissionEntity : this.getRolePermissions()) { + PermissionEntity permissionEntity = rolePermissionEntity.getPermission(); + WildcardPermission rolePermission = new WildcardPermission(permissionEntity.getValue()); + if (rolePermission.implies(p)) { + return true; + } + } + return false; + } }