Skip to content

Commit caec537

Browse files
oleg-odysseusoleg-odysseus
committed
Eliminated duplicated permission check operations
1 parent fe06d61 commit caec537

9 files changed

+84
-89
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;
@@ -29,6 +31,7 @@
2931
import org.ohdsi.webapi.reusable.domain.Reusable;
3032
import org.ohdsi.webapi.security.PermissionService;
3133
import org.ohdsi.webapi.service.dto.CommonEntityDTO;
34+
import org.ohdsi.webapi.service.dto.PermissionCheckType;
3235
import org.ohdsi.webapi.shiro.Entities.UserEntity;
3336
import org.ohdsi.webapi.shiro.Entities.UserRepository;
3437
import org.ohdsi.webapi.shiro.management.DisabledSecurity;
@@ -371,7 +374,7 @@ protected PermissionService getPermissionService() {
371374
}
372375

373376
protected void assignTag(CommonEntityExt<?> entity, int tagId) {
374-
checkOwnerOrAdminOrGrantedOrTagManager(entity);
377+
checkPermissions(entity, ImmutableSet.of(PermissionCheckType.IS_OWNER, PermissionCheckType.IS_ADMIN, PermissionCheckType.HAS_WRITE_ACCESS, PermissionCheckType.IS_TAG_MANAGER));
375378
if (Objects.nonNull(entity)) {
376379
Tag tag = tagService.getById(tagId);
377380
if (Objects.nonNull(tag)) {
@@ -398,7 +401,7 @@ protected void assignTag(CommonEntityExt<?> entity, int tagId) {
398401
}
399402

400403
protected void unassignTag(CommonEntityExt<?> entity, int tagId) {
401-
checkOwnerOrAdminOrGrantedOrTagManager(entity);
404+
checkPermissions(entity, ImmutableSet.of(PermissionCheckType.IS_OWNER, PermissionCheckType.IS_ADMIN, PermissionCheckType.HAS_WRITE_ACCESS, PermissionCheckType.IS_TAG_MANAGER));
402405
if (Objects.nonNull(entity)) {
403406
Tag tag = tagService.getById(tagId);
404407
if (Objects.nonNull(tag)) {
@@ -422,56 +425,30 @@ private boolean hasPermissionToAssignProtectedTags(final CommonEntityExt<?> enti
422425
return TagSecurityUtils.checkPermission(TagSecurityUtils.getAssetName(entity), method);
423426
}
424427

425-
protected void checkOwnerOrAdmin(UserEntity owner) {
426-
if (security instanceof DisabledSecurity) {
428+
protected void checkPermissions(CommonEntity<?> entity, Set<PermissionCheckType> permissionsToCheck) {
429+
if(CollectionUtils.isEmpty(permissionsToCheck)){
427430
return;
428431
}
429432

430-
UserEntity user = getCurrentUser();
431-
Long ownerId = Objects.nonNull(owner) ? owner.getId() : null;
432-
433-
if (!(user.getId().equals(ownerId) || isAdmin())) {
434-
throw new ForbiddenException();
435-
}
436-
}
437-
438-
protected void checkOwnerOrAdminOrModerator(UserEntity owner) {
439-
if (security instanceof DisabledSecurity) {
440-
return;
441-
}
442-
443-
UserEntity user = getCurrentUser();
444-
Long ownerId = Objects.nonNull(owner) ? owner.getId() : null;
445-
446-
if (!(user.getId().equals(ownerId) || isAdmin() || isModerator())) {
447-
throw new ForbiddenException();
448-
}
449-
}
450-
451-
protected void checkOwnerOrAdminOrGranted(CommonEntity<?> entity) {
452433
if (security instanceof DisabledSecurity) {
453434
return;
454435
}
455436

456-
UserEntity user = getCurrentUser();
457-
Long ownerId = Objects.nonNull(entity.getCreatedBy()) ? entity.getCreatedBy().getId() : null;
437+
boolean isAllowed = (permissionsToCheck.contains(PermissionCheckType.IS_OWNER) && isEntityOwner(entity)) ||
438+
(permissionsToCheck.contains(PermissionCheckType.IS_ADMIN) && isAdmin()) ||
439+
(permissionsToCheck.contains(PermissionCheckType.IS_MODERATOR) && isModerator()) ||
440+
(permissionsToCheck.contains(PermissionCheckType.HAS_WRITE_ACCESS) && permissionService.hasWriteAccess(entity)) ||
441+
(permissionsToCheck.contains(PermissionCheckType.IS_TAG_MANAGER) && TagSecurityUtils.canManageTags());
458442

459-
if (!(user.getId().equals(ownerId) || isAdmin() || permissionService.hasWriteAccess(entity))) {
443+
if(!isAllowed) {
460444
throw new ForbiddenException();
461445
}
462446
}
463447

464-
protected void checkOwnerOrAdminOrGrantedOrTagManager(CommonEntity<?> entity) {
465-
if (security instanceof DisabledSecurity) {
466-
return;
467-
}
468-
448+
private boolean isEntityOwner(CommonEntity<?> entity){
469449
UserEntity user = getCurrentUser();
470450
Long ownerId = Objects.nonNull(entity.getCreatedBy()) ? entity.getCreatedBy().getId() : null;
471-
472-
if (!(user.getId().equals(ownerId) || isAdmin() || permissionService.hasWriteAccess(entity) || TagSecurityUtils.canManageTags())) {
473-
throw new ForbiddenException();
474-
}
451+
return user.getId().equals(ownerId);
475452
}
476453

477454
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;
@@ -72,6 +73,7 @@
7273
import org.ohdsi.webapi.job.JobTemplate;
7374
import org.ohdsi.webapi.security.PermissionService;
7475
import org.ohdsi.webapi.service.dto.CheckResultDTO;
76+
import org.ohdsi.webapi.service.dto.PermissionCheckType;
7577
import org.ohdsi.webapi.shiro.Entities.UserEntity;
7678
import org.ohdsi.webapi.shiro.management.datasource.SourceIdAccessor;
7779
import org.ohdsi.webapi.source.Source;
@@ -1463,7 +1465,7 @@ private void checkVersion(int id, int version, boolean checkOwnerShip) {
14631465

14641466
CohortDefinition entity = cohortDefinitionRepository.findOne(id);
14651467
if (checkOwnerShip) {
1466-
checkOwnerOrAdminOrGranted(entity);
1468+
checkPermissions(entity, ImmutableSet.of(PermissionCheckType.IS_OWNER, PermissionCheckType.IS_ADMIN, PermissionCheckType.HAS_WRITE_ACCESS));
14671469
}
14681470
}
14691471

0 commit comments

Comments
 (0)