From d18455d1324a23d03ae7edff977d54a551cb9e7e Mon Sep 17 00:00:00 2001 From: oleg-odysseus Date: Mon, 14 Oct 2024 17:53:09 +0200 Subject: [PATCH] [issue-2953] Fixed access to resources for non-admin Tag Managers (having a 'tag:management' permission) --- .../cohortcharacterization/CcServiceImpl.java | 2 -- .../webapi/pathway/PathwayServiceImpl.java | 26 +++++++++---------- .../webapi/service/AbstractDaoService.java | 15 +++++++++++ .../service/CohortDefinitionService.java | 2 -- .../webapi/service/ConceptSetService.java | 2 -- .../webapi/service/IRAnalysisService.java | 2 -- .../org/ohdsi/webapi/tag/TagGroupService.java | 1 + .../ohdsi/webapi/tag/TagSecurityUtils.java | 4 +++ .../java/org/ohdsi/webapi/tag/TagService.java | 2 +- 9 files changed, 33 insertions(+), 23 deletions(-) diff --git a/src/main/java/org/ohdsi/webapi/cohortcharacterization/CcServiceImpl.java b/src/main/java/org/ohdsi/webapi/cohortcharacterization/CcServiceImpl.java index a10b3a8ff9..97ce5ea87f 100644 --- a/src/main/java/org/ohdsi/webapi/cohortcharacterization/CcServiceImpl.java +++ b/src/main/java/org/ohdsi/webapi/cohortcharacterization/CcServiceImpl.java @@ -322,7 +322,6 @@ public void onFeAnalysisChanged(FeAnalysisChangedEvent event) { @Transactional public void assignTag(Long id, int tagId) { CohortCharacterizationEntity entity = findById(id); - checkOwnerOrAdminOrGranted(entity); assignTag(entity, tagId); } @@ -330,7 +329,6 @@ public void assignTag(Long id, int tagId) { @Transactional public void unassignTag(Long id, int tagId) { CohortCharacterizationEntity entity = findById(id); - checkOwnerOrAdminOrGranted(entity); unassignTag(entity, tagId); } diff --git a/src/main/java/org/ohdsi/webapi/pathway/PathwayServiceImpl.java b/src/main/java/org/ohdsi/webapi/pathway/PathwayServiceImpl.java index dd4e62987f..52bb7ba177 100644 --- a/src/main/java/org/ohdsi/webapi/pathway/PathwayServiceImpl.java +++ b/src/main/java/org/ohdsi/webapi/pathway/PathwayServiceImpl.java @@ -534,19 +534,17 @@ public String findDesignByGenerationId(@PathwayAnalysisGenerationId final Long i return entity.getDesign(); } - @Override - public void assignTag(Integer id, int tagId) { - PathwayAnalysisEntity entity = getById(id); - checkOwnerOrAdminOrGranted(entity); - assignTag(entity, tagId); - } - - @Override - public void unassignTag(Integer id, int tagId) { - PathwayAnalysisEntity entity = getById(id); - checkOwnerOrAdminOrGranted(entity); - unassignTag(entity, tagId); - } + @Override + public void assignTag(Integer id, int tagId) { + PathwayAnalysisEntity entity = getById(id); + assignTag(entity, tagId); + } + + @Override + public void unassignTag(Integer id, int tagId) { + PathwayAnalysisEntity entity = getById(id); + unassignTag(entity, tagId); + } @Override public List getVersions(long id) { @@ -657,7 +655,7 @@ private PathwayAnalysisResult queryGenerationResults(Source source, Long generat new String[]{GENERATION_ID}, new Object[]{generationId} ); - Map> pathwayResults = + Map> pathwayResults = getSourceJdbcTemplate(source).query(pathwayResultsPsr.getSql(), pathwayResultsPsr.getOrderedParams(), pathwayExtractor); cohortStats.stream().forEach((cp) -> { diff --git a/src/main/java/org/ohdsi/webapi/service/AbstractDaoService.java b/src/main/java/org/ohdsi/webapi/service/AbstractDaoService.java index 95277cbe68..d4b86dd1d8 100644 --- a/src/main/java/org/ohdsi/webapi/service/AbstractDaoService.java +++ b/src/main/java/org/ohdsi/webapi/service/AbstractDaoService.java @@ -364,6 +364,7 @@ protected PermissionService getPermissionService() { } protected void assignTag(CommonEntityExt entity, int tagId) { + checkOwnerOrAdminOrGrantedOrTagManager(entity); if (Objects.nonNull(entity)) { Tag tag = tagService.getById(tagId); if (Objects.nonNull(tag)) { @@ -390,6 +391,7 @@ protected void assignTag(CommonEntityExt entity, int tagId) { } protected void unassignTag(CommonEntityExt entity, int tagId) { + checkOwnerOrAdminOrGrantedOrTagManager(entity); if (Objects.nonNull(entity)) { Tag tag = tagService.getById(tagId); if (Objects.nonNull(tag)) { @@ -452,6 +454,19 @@ protected void checkOwnerOrAdminOrGranted(CommonEntity entity) { } } + protected void checkOwnerOrAdminOrGrantedOrTagManager(CommonEntity entity) { + if (security instanceof DisabledSecurity) { + return; + } + + UserEntity user = getCurrentUser(); + Long ownerId = Objects.nonNull(entity.getCreatedBy()) ? entity.getCreatedBy().getId() : null; + + if (!(user.getId().equals(ownerId) || isAdmin() || permissionService.hasWriteAccess(entity) || TagSecurityUtils.canManageTags())) { + throw new ForbiddenException(); + } + } + protected List listByTags(List> entities, List names, Class clazz) { diff --git a/src/main/java/org/ohdsi/webapi/service/CohortDefinitionService.java b/src/main/java/org/ohdsi/webapi/service/CohortDefinitionService.java index 4de4e872e6..7e5924a6b5 100644 --- a/src/main/java/org/ohdsi/webapi/service/CohortDefinitionService.java +++ b/src/main/java/org/ohdsi/webapi/service/CohortDefinitionService.java @@ -957,7 +957,6 @@ private Response printFrindly(String markdown, String format) { @Transactional public void assignTag(@PathParam("id") final Integer id, final int tagId) { CohortDefinition entity = cohortDefinitionRepository.findOne(id); - checkOwnerOrAdminOrGranted(entity); assignTag(entity, tagId); } @@ -974,7 +973,6 @@ public void assignTag(@PathParam("id") final Integer id, final int tagId) { @Transactional public void unassignTag(@PathParam("id") final Integer id, @PathParam("tagId") final int tagId) { CohortDefinition entity = cohortDefinitionRepository.findOne(id); - checkOwnerOrAdminOrGranted(entity); unassignTag(entity, tagId); } diff --git a/src/main/java/org/ohdsi/webapi/service/ConceptSetService.java b/src/main/java/org/ohdsi/webapi/service/ConceptSetService.java index 2f80ef991e..4a323532a0 100644 --- a/src/main/java/org/ohdsi/webapi/service/ConceptSetService.java +++ b/src/main/java/org/ohdsi/webapi/service/ConceptSetService.java @@ -628,7 +628,6 @@ public void deleteConceptSet(@PathParam("id") final int id) { @Transactional public void assignTag(@PathParam("id") final Integer id, final int tagId) { ConceptSet entity = getConceptSetRepository().findById(id); - checkOwnerOrAdminOrGranted(entity); assignTag(entity, tagId); } @@ -646,7 +645,6 @@ public void assignTag(@PathParam("id") final Integer id, final int tagId) { @Transactional public void unassignTag(@PathParam("id") final Integer id, @PathParam("tagId") final int tagId) { ConceptSet entity = getConceptSetRepository().findById(id); - checkOwnerOrAdminOrGranted(entity); unassignTag(entity, tagId); } diff --git a/src/main/java/org/ohdsi/webapi/service/IRAnalysisService.java b/src/main/java/org/ohdsi/webapi/service/IRAnalysisService.java index 342558b823..9e1d03e7e0 100644 --- a/src/main/java/org/ohdsi/webapi/service/IRAnalysisService.java +++ b/src/main/java/org/ohdsi/webapi/service/IRAnalysisService.java @@ -806,7 +806,6 @@ public void deleteInfo(final int id, final String sourceKey) { @Transactional public void assignTag(final Integer id, final int tagId) { IncidenceRateAnalysis entity = irAnalysisRepository.findOne(id); - checkOwnerOrAdminOrGranted(entity); assignTag(entity, tagId); } @@ -814,7 +813,6 @@ public void assignTag(final Integer id, final int tagId) { @Transactional public void unassignTag(final Integer id, final int tagId) { IncidenceRateAnalysis entity = irAnalysisRepository.findOne(id); - checkOwnerOrAdminOrGranted(entity); unassignTag(entity, tagId); } diff --git a/src/main/java/org/ohdsi/webapi/tag/TagGroupService.java b/src/main/java/org/ohdsi/webapi/tag/TagGroupService.java index 93f98cab0b..873cbc9fb6 100644 --- a/src/main/java/org/ohdsi/webapi/tag/TagGroupService.java +++ b/src/main/java/org/ohdsi/webapi/tag/TagGroupService.java @@ -75,6 +75,7 @@ private void assignGroup(HasTags service, List assetIds service.assignTag(id, tagId); } catch (final ForbiddenException e) { log.warn("Tag {} cannot be assigned to entity {} in service {} - forbidden", tagId, id, service.getClass().getName()); + throw e; } }); } diff --git a/src/main/java/org/ohdsi/webapi/tag/TagSecurityUtils.java b/src/main/java/org/ohdsi/webapi/tag/TagSecurityUtils.java index ec04c877d8..e2a6719d5b 100644 --- a/src/main/java/org/ohdsi/webapi/tag/TagSecurityUtils.java +++ b/src/main/java/org/ohdsi/webapi/tag/TagSecurityUtils.java @@ -74,4 +74,8 @@ public static String getAssetName(final CommonEntityExt entity) { } return null; } + + public static boolean canManageTags() { + return SecurityUtils.getSubject().isPermitted("tag:management"); + } } diff --git a/src/main/java/org/ohdsi/webapi/tag/TagService.java b/src/main/java/org/ohdsi/webapi/tag/TagService.java index c7e3f5abc7..85c50dedb8 100644 --- a/src/main/java/org/ohdsi/webapi/tag/TagService.java +++ b/src/main/java/org/ohdsi/webapi/tag/TagService.java @@ -65,7 +65,7 @@ public Tag create(Tag tag) { .filter(Tag::isAllowCustom) .count() == groups.size(); - if (this.getPermissionService().isSecurityEnabled() && !SecurityUtils.getSubject().isPermitted("tag:management") && !allowCustom) { + if (this.getPermissionService().isSecurityEnabled() && !TagSecurityUtils.canManageTags() && !allowCustom) { throw new IllegalArgumentException("Tag can be added only to groups that allows to do it"); }