Skip to content

Commit ec314b2

Browse files
oleg-odysseusoleg-odysseus
committed
Eliminated duplicated permission check operations
1 parent e962858 commit ec314b2

9 files changed

+94
-98
lines changed

‎src/main/java/org/ohdsi/webapi/cohortcharacterization/CcServiceImpl.java

+3-1
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import com.cosium.spring.data.jpa.entity.graph.domain.EntityGraph;
44
import com.fasterxml.jackson.core.type.TypeReference;
55
import com.google.common.collect.ImmutableList;
6+
import com.google.common.collect.ImmutableSet;
67
import org.apache.commons.io.FileUtils;
78
import org.apache.commons.lang3.StringUtils;
89
import org.ohdsi.analysis.Utils;
@@ -60,6 +61,7 @@
6061
import org.ohdsi.webapi.service.FeatureExtractionService;
6162
import org.ohdsi.webapi.service.JobService;
6263
import org.ohdsi.webapi.service.VocabularyService;
64+
import org.ohdsi.webapi.service.dto.PermissionCheckType;
6365
import org.ohdsi.webapi.shiro.Entities.UserEntity;
6466
import org.ohdsi.webapi.shiro.annotations.CcGenerationId;
6567
import org.ohdsi.webapi.shiro.annotations.DataSourceAccess;
@@ -1024,7 +1026,7 @@ private void checkVersion(long id, int version, boolean checkOwnerShip) {
10241026

10251027
CohortCharacterizationEntity entity = findById(id);
10261028
if (checkOwnerShip) {
1027-
checkOwnerOrAdminOrGranted(entity);
1029+
checkPermissions(entity, ImmutableSet.of(PermissionCheckType.IS_OWNER, PermissionCheckType.IS_ADMIN, PermissionCheckType.HAS_WRITE_ACCESS));
10281030
}
10291031
}
10301032

‎src/main/java/org/ohdsi/webapi/pathway/PathwayServiceImpl.java

+7-5
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import com.cosium.spring.data.jpa.entity.graph.domain.EntityGraph;
44
import com.google.common.base.MoreObjects;
5+
import com.google.common.collect.ImmutableSet;
56
import com.odysseusinc.arachne.commons.types.DBMSType;
67
import org.hibernate.Hibernate;
78
import org.ohdsi.circe.helper.ResourceHelper;
@@ -33,6 +34,7 @@
3334
import org.ohdsi.webapi.service.AbstractDaoService;
3435
import org.ohdsi.webapi.service.CohortDefinitionService;
3536
import org.ohdsi.webapi.service.JobService;
37+
import org.ohdsi.webapi.service.dto.PermissionCheckType;
3638
import org.ohdsi.webapi.shiro.Entities.UserEntity;
3739
import org.ohdsi.webapi.shiro.Entities.UserRepository;
3840
import org.ohdsi.webapi.shiro.annotations.DataSourceAccess;
@@ -611,11 +613,11 @@ private void checkVersion(int id, int version, boolean checkOwnerShip) {
611613
ExceptionUtils.throwNotFoundExceptionIfNull(pathwayVersion,
612614
String.format("There is no pathway analysis version with id = %d.", version));
613615

614-
PathwayAnalysisEntity entity = this.pathwayAnalysisRepository.findOne(id);
615-
if (checkOwnerShip) {
616-
checkOwnerOrAdminOrGranted(entity);
617-
}
618-
}
616+
PathwayAnalysisEntity entity = this.pathwayAnalysisRepository.findOne(id);
617+
if (checkOwnerShip) {
618+
checkPermissions(entity, ImmutableSet.of(PermissionCheckType.IS_OWNER, PermissionCheckType.IS_ADMIN, PermissionCheckType.HAS_WRITE_ACCESS));
619+
}
620+
}
619621

620622
public PathwayVersion saveVersion(int id) {
621623
PathwayAnalysisEntity def = this.pathwayAnalysisRepository.findOne(id);

‎src/main/java/org/ohdsi/webapi/reusable/ReusableService.java

+4-4
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
package org.ohdsi.webapi.reusable;
22

3+
import com.google.common.collect.ImmutableSet;
34
import org.ohdsi.webapi.reusable.domain.Reusable;
45
import org.ohdsi.webapi.reusable.dto.ReusableDTO;
56
import org.ohdsi.webapi.reusable.dto.ReusableVersionFullDTO;
67
import org.ohdsi.webapi.reusable.repository.ReusableRepository;
78
import org.ohdsi.webapi.security.PermissionService;
89
import org.ohdsi.webapi.service.AbstractDaoService;
10+
import org.ohdsi.webapi.service.dto.PermissionCheckType;
911
import org.ohdsi.webapi.shiro.Entities.UserEntity;
1012
import org.ohdsi.webapi.tag.domain.HasTags;
1113
import org.ohdsi.webapi.tag.dto.TagNameListRequestDTO;
@@ -132,9 +134,7 @@ public void unassignTag(Integer id, int tagId) {
132134

133135
public void delete(Integer id) {
134136
Reusable existing = reusableRepository.findOne(id);
135-
136-
checkOwnerOrAdminOrModerator(existing.getCreatedBy());
137-
137+
checkPermissions(existing, ImmutableSet.of(PermissionCheckType.IS_OWNER, PermissionCheckType.IS_ADMIN, PermissionCheckType.IS_MODERATOR));
138138
reusableRepository.delete(id);
139139
}
140140

@@ -197,7 +197,7 @@ private void checkVersion(int id, int version, boolean checkOwnerShip) {
197197

198198
Reusable entity = this.reusableRepository.findOne(id);
199199
if (checkOwnerShip) {
200-
checkOwnerOrAdminOrGranted(entity);
200+
checkPermissions(entity, ImmutableSet.of(PermissionCheckType.IS_OWNER, PermissionCheckType.IS_ADMIN, PermissionCheckType.HAS_WRITE_ACCESS));
201201
}
202202
}
203203

‎src/main/java/org/ohdsi/webapi/service/AbstractDaoService.java

+15-38
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
package org.ohdsi.webapi.service;
22

3+
import com.google.common.collect.ImmutableSet;
34
import com.odysseusinc.arachne.commons.types.DBMSType;
45
import com.odysseusinc.arachne.execution_engine_common.api.v1.dto.DataSourceUnsecuredDTO;
56
import com.odysseusinc.datasourcemanager.krblogin.KerberosService;
67
import com.odysseusinc.datasourcemanager.krblogin.KrbConfig;
78
import com.odysseusinc.datasourcemanager.krblogin.RuntimeServiceMode;
9+
import org.apache.commons.collections4.CollectionUtils;
810
import org.apache.commons.io.FileUtils;
911
import org.apache.commons.lang3.StringUtils;
1012
import org.apache.shiro.SecurityUtils;
@@ -28,6 +30,7 @@
2830
import org.ohdsi.webapi.reusable.domain.Reusable;
2931
import org.ohdsi.webapi.security.PermissionService;
3032
import org.ohdsi.webapi.service.dto.CommonEntityDTO;
33+
import org.ohdsi.webapi.service.dto.PermissionCheckType;
3134
import org.ohdsi.webapi.shiro.Entities.UserEntity;
3235
import org.ohdsi.webapi.shiro.Entities.UserRepository;
3336
import org.ohdsi.webapi.shiro.management.DisabledSecurity;
@@ -364,7 +367,7 @@ protected PermissionService getPermissionService() {
364367
}
365368

366369
protected void assignTag(CommonEntityExt<?> entity, int tagId) {
367-
checkOwnerOrAdminOrGrantedOrTagManager(entity);
370+
checkPermissions(entity, ImmutableSet.of(PermissionCheckType.IS_OWNER, PermissionCheckType.IS_ADMIN, PermissionCheckType.HAS_WRITE_ACCESS, PermissionCheckType.IS_TAG_MANAGER));
368371
if (Objects.nonNull(entity)) {
369372
Tag tag = tagService.getById(tagId);
370373
if (Objects.nonNull(tag)) {
@@ -391,7 +394,7 @@ protected void assignTag(CommonEntityExt<?> entity, int tagId) {
391394
}
392395

393396
protected void unassignTag(CommonEntityExt<?> entity, int tagId) {
394-
checkOwnerOrAdminOrGrantedOrTagManager(entity);
397+
checkPermissions(entity, ImmutableSet.of(PermissionCheckType.IS_OWNER, PermissionCheckType.IS_ADMIN, PermissionCheckType.HAS_WRITE_ACCESS, PermissionCheckType.IS_TAG_MANAGER));
395398
if (Objects.nonNull(entity)) {
396399
Tag tag = tagService.getById(tagId);
397400
if (Objects.nonNull(tag)) {
@@ -415,56 +418,30 @@ private boolean hasPermissionToAssignProtectedTags(final CommonEntityExt<?> enti
415418
return TagSecurityUtils.checkPermission(TagSecurityUtils.getAssetName(entity), method);
416419
}
417420

418-
protected void checkOwnerOrAdmin(UserEntity owner) {
419-
if (security instanceof DisabledSecurity) {
421+
protected void checkPermissions(CommonEntity<?> entity, Set<PermissionCheckType> permissionsToCheck) {
422+
if(CollectionUtils.isEmpty(permissionsToCheck)){
420423
return;
421424
}
422425

423-
UserEntity user = getCurrentUser();
424-
Long ownerId = Objects.nonNull(owner) ? owner.getId() : null;
425-
426-
if (!(user.getId().equals(ownerId) || isAdmin())) {
427-
throw new ForbiddenException();
428-
}
429-
}
430-
431-
protected void checkOwnerOrAdminOrModerator(UserEntity owner) {
432-
if (security instanceof DisabledSecurity) {
433-
return;
434-
}
435-
436-
UserEntity user = getCurrentUser();
437-
Long ownerId = Objects.nonNull(owner) ? owner.getId() : null;
438-
439-
if (!(user.getId().equals(ownerId) || isAdmin() || isModerator())) {
440-
throw new ForbiddenException();
441-
}
442-
}
443-
444-
protected void checkOwnerOrAdminOrGranted(CommonEntity<?> entity) {
445426
if (security instanceof DisabledSecurity) {
446427
return;
447428
}
448429

449-
UserEntity user = getCurrentUser();
450-
Long ownerId = Objects.nonNull(entity.getCreatedBy()) ? entity.getCreatedBy().getId() : null;
430+
boolean isAllowed = (permissionsToCheck.contains(PermissionCheckType.IS_OWNER) && isEntityOwner(entity)) ||
431+
(permissionsToCheck.contains(PermissionCheckType.IS_ADMIN) && isAdmin()) ||
432+
(permissionsToCheck.contains(PermissionCheckType.IS_MODERATOR) && isModerator()) ||
433+
(permissionsToCheck.contains(PermissionCheckType.HAS_WRITE_ACCESS) && permissionService.hasWriteAccess(entity)) ||
434+
(permissionsToCheck.contains(PermissionCheckType.IS_TAG_MANAGER) && TagSecurityUtils.canManageTags());
451435

452-
if (!(user.getId().equals(ownerId) || isAdmin() || permissionService.hasWriteAccess(entity))) {
436+
if(!isAllowed) {
453437
throw new ForbiddenException();
454438
}
455439
}
456440

457-
protected void checkOwnerOrAdminOrGrantedOrTagManager(CommonEntity<?> entity) {
458-
if (security instanceof DisabledSecurity) {
459-
return;
460-
}
461-
441+
private boolean isEntityOwner(CommonEntity<?> entity){
462442
UserEntity user = getCurrentUser();
463443
Long ownerId = Objects.nonNull(entity.getCreatedBy()) ? entity.getCreatedBy().getId() : null;
464-
465-
if (!(user.getId().equals(ownerId) || isAdmin() || permissionService.hasWriteAccess(entity) || TagSecurityUtils.canManageTags())) {
466-
throw new ForbiddenException();
467-
}
444+
return user.getId().equals(ownerId);
468445
}
469446

470447
protected <T extends CommonEntityDTO> List<T> listByTags(List<? extends CommonEntityExt<? extends Number>> entities,

‎src/main/java/org/ohdsi/webapi/service/CohortDefinitionService.java

+3-1
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import com.fasterxml.jackson.annotation.JsonProperty;
2020
import com.fasterxml.jackson.databind.ObjectMapper;
21+
import com.google.common.collect.ImmutableSet;
2122
import org.apache.commons.lang3.StringUtils;
2223
import org.commonmark.Extension;
2324
import org.commonmark.ext.gfm.tables.TablesExtension;
@@ -59,6 +60,7 @@
5960
import org.ohdsi.webapi.job.JobTemplate;
6061
import org.ohdsi.webapi.security.PermissionService;
6162
import org.ohdsi.webapi.service.dto.CheckResultDTO;
63+
import org.ohdsi.webapi.service.dto.PermissionCheckType;
6264
import org.ohdsi.webapi.shiro.Entities.UserEntity;
6365
import org.ohdsi.webapi.shiro.management.datasource.SourceIdAccessor;
6466
import org.ohdsi.webapi.source.Source;
@@ -1153,7 +1155,7 @@ private void checkVersion(int id, int version, boolean checkOwnerShip) {
11531155

11541156
CohortDefinition entity = cohortDefinitionRepository.findOne(id);
11551157
if (checkOwnerShip) {
1156-
checkOwnerOrAdminOrGranted(entity);
1158+
checkPermissions(entity, ImmutableSet.of(PermissionCheckType.IS_OWNER, PermissionCheckType.IS_ADMIN, PermissionCheckType.HAS_WRITE_ACCESS));
11571159
}
11581160
}
11591161

0 commit comments

Comments
 (0)