From 6cc2f7b84b757ee97f2614b0982f3801cc6db6bd Mon Sep 17 00:00:00 2001 From: anhefti Date: Wed, 26 Jun 2024 12:59:48 +0200 Subject: [PATCH] check right privileges batch actions and code cleanup --- .../seb/sebserver/gbl/model/BatchAction.java | 4 +-- .../SEBExamConfigBatchStateChangePopup.java | 2 +- .../authorization/AuthorizationService.java | 31 +++++++++++++++++-- .../authorization/UserService.java | 8 ++++- .../impl/AuthorizationServiceImpl.java | 6 ++++ .../authorization/impl/UserServiceImpl.java | 22 ++++++++++++- .../bulkaction/BatchActionExec.java | 2 +- .../bulkaction/BatchActionService.java | 2 +- .../bulkaction/impl/DeleteExamAction.java | 5 +-- .../bulkaction/impl/ExamConfigDelete.java | 19 +++++++----- .../impl/ExamConfigStateChange.java | 2 +- src/main/resources/messages.properties | 2 ++ .../integration/UseCasesIntegrationTest.java | 2 +- 13 files changed, 87 insertions(+), 20 deletions(-) diff --git a/src/main/java/ch/ethz/seb/sebserver/gbl/model/BatchAction.java b/src/main/java/ch/ethz/seb/sebserver/gbl/model/BatchAction.java index 9a480b00..89e38e4e 100644 --- a/src/main/java/ch/ethz/seb/sebserver/gbl/model/BatchAction.java +++ b/src/main/java/ch/ethz/seb/sebserver/gbl/model/BatchAction.java @@ -34,10 +34,10 @@ public class BatchAction implements GrantEntity { public static final String ATTR_FAILURES = "failures"; public static final String FINISHED_FLAG = "_FINISHED"; - public static final String ACTION_ATTRIBUT_TARGET_STATE = "batchActionTargetState"; + public static final String ACTION_ATTRIBUTE_TARGET_STATE = "batchActionTargetState"; private static final Set ACTION_ATTRIBUTES = new HashSet<>(Arrays.asList( - ACTION_ATTRIBUT_TARGET_STATE)); + ACTION_ATTRIBUTE_TARGET_STATE)); @JsonProperty(BATCH_ACTION.ATTR_ID) public final Long id; diff --git a/src/main/java/ch/ethz/seb/sebserver/gui/content/configs/SEBExamConfigBatchStateChangePopup.java b/src/main/java/ch/ethz/seb/sebserver/gui/content/configs/SEBExamConfigBatchStateChangePopup.java index 2a8d76f6..82eebfc8 100644 --- a/src/main/java/ch/ethz/seb/sebserver/gui/content/configs/SEBExamConfigBatchStateChangePopup.java +++ b/src/main/java/ch/ethz/seb/sebserver/gui/content/configs/SEBExamConfigBatchStateChangePopup.java @@ -112,7 +112,7 @@ public class SEBExamConfigBatchStateChangePopup extends AbstractBatchActionWizar throw new IllegalArgumentException("missing " + ATTR_SELECTED_TARGET_STATE + " from pageContext"); } - batchActionRequestBuilder.withFormParam(BatchAction.ACTION_ATTRIBUT_TARGET_STATE, targetStateName); + batchActionRequestBuilder.withFormParam(BatchAction.ACTION_ATTRIBUTE_TARGET_STATE, targetStateName); } @Override diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/authorization/AuthorizationService.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/authorization/AuthorizationService.java index f6d5cd96..4470b3e2 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/authorization/AuthorizationService.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/authorization/AuthorizationService.java @@ -18,11 +18,11 @@ import ch.ethz.seb.sebserver.gbl.api.EntityType; import ch.ethz.seb.sebserver.gbl.api.authorization.Privilege; import ch.ethz.seb.sebserver.gbl.api.authorization.PrivilegeType; import ch.ethz.seb.sebserver.gbl.model.GrantEntity; -import ch.ethz.seb.sebserver.gbl.model.user.UserFeatures; import ch.ethz.seb.sebserver.gbl.model.user.UserInfo; import ch.ethz.seb.sebserver.gbl.model.user.UserRole; import ch.ethz.seb.sebserver.gbl.util.Result; import ch.ethz.seb.sebserver.webservice.servicelayer.authorization.impl.SEBServerUser; +import ch.ethz.seb.sebserver.webservice.servicelayer.dao.ResourceNotFoundException; /** A service to check authorization grants for a given user for entity-types and -instances *

@@ -249,6 +249,34 @@ public interface AuthorizationService { return check(PrivilegeType.WRITE, entity); } + default Result checkForUser( + final PrivilegeType privilegeType, + final E entity, + final String ownerId) { + + final UserInfo userInfo = getUserService().getUser(ownerId); + if (userInfo == null) { + return Result.ofError(new ResourceNotFoundException(EntityType.USER, ownerId)); + } + + if (!hasGrant( + privilegeType, + entity.entityType(), + entity.getInstitutionId(), + entity.getOwnerId(), + userInfo.uuid, + userInfo.institutionId, + userInfo.getUserRoles())) { + + throw new PermissionDeniedException( + entity, + privilegeType, + getUserService().getCurrentUser().getUserInfo().uuid); + } + + return Result.of(entity); + } + /** Checks if the current user has a specified role. * If not a PermissionDeniedException is thrown for the given EntityType * @@ -274,5 +302,4 @@ public interface AuthorizationService { currentUser.getUserInfo()); } } - } diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/authorization/UserService.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/authorization/UserService.java index 6cb5d651..fde10800 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/authorization/UserService.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/authorization/UserService.java @@ -10,7 +10,7 @@ package ch.ethz.seb.sebserver.webservice.servicelayer.authorization; import java.security.Principal; -import ch.ethz.seb.sebserver.gbl.model.user.UserFeatures; +import ch.ethz.seb.sebserver.gbl.model.user.UserInfo; import org.springframework.security.core.Authentication; import org.springframework.web.bind.WebDataBinder; @@ -60,4 +60,10 @@ public interface UserService { * @param authentication the Authentication context*/ void setAuthenticationIfAbsent(Authentication authentication); + /** Gets the user for model id / uuid or null if not exists or not active + * + * @param userId The user uuid + * @return UserInfo for the user + */ + UserInfo getUser(String userId); } diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/authorization/impl/AuthorizationServiceImpl.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/authorization/impl/AuthorizationServiceImpl.java index 124f0fd5..3b95152a 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/authorization/impl/AuthorizationServiceImpl.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/authorization/impl/AuthorizationServiceImpl.java @@ -16,6 +16,10 @@ import java.util.Set; import javax.annotation.PostConstruct; +import ch.ethz.seb.sebserver.gbl.model.GrantEntity; +import ch.ethz.seb.sebserver.gbl.model.exam.Exam; +import ch.ethz.seb.sebserver.gbl.util.Result; +import ch.ethz.seb.sebserver.webservice.servicelayer.authorization.PermissionDeniedException; import org.springframework.context.annotation.Lazy; import org.springframework.stereotype.Service; @@ -279,6 +283,8 @@ public class AuthorizationServiceImpl implements AuthorizationService { } + + private PrivilegeBuilder addPrivilege(final EntityType entityType) { return new PrivilegeBuilder(entityType); } diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/authorization/impl/UserServiceImpl.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/authorization/impl/UserServiceImpl.java index c7aafda2..d04d06fd 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/authorization/impl/UserServiceImpl.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/authorization/impl/UserServiceImpl.java @@ -15,6 +15,7 @@ import java.util.Collection; import java.util.Collections; import java.util.stream.Collectors; +import ch.ethz.seb.sebserver.webservice.servicelayer.dao.UserDAO; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.context.annotation.Lazy; @@ -37,6 +38,7 @@ import ch.ethz.seb.sebserver.webservice.servicelayer.authorization.UserService; public class UserServiceImpl implements UserService { private static final Logger log = LoggerFactory.getLogger(UserServiceImpl.class); + private final UserDAO userDAO; public interface ExtractUserFromAuthenticationStrategy { @@ -45,8 +47,11 @@ public class UserServiceImpl implements UserService { private final Collection extractStrategies; - public UserServiceImpl(final Collection extractStrategies) { + public UserServiceImpl( + final UserDAO userDAO, + final Collection extractStrategies) { + this.userDAO = userDAO; this.extractStrategies = extractStrategies; } @@ -108,6 +113,21 @@ public class UserServiceImpl implements UserService { } } + @Override + public UserInfo getUser(final String userId) { + final UserInfo user = this.userDAO + .byModelId(userId) + .onError(error -> log.error("Failed to find user for id: {} error: {}", userId, error.getMessage())) + .getOr(null); + + if (user != null && !user.isActive()) { + log.warn("Try to get inactive user for processing: {}", userId); + return null; + } + + return user; + } + // 1. OAuth2Authentication strategy @Lazy @Component diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/BatchActionExec.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/BatchActionExec.java index c348971e..27d55c9a 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/BatchActionExec.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/BatchActionExec.java @@ -29,7 +29,7 @@ public interface BatchActionExec { * This shall check whether the needed attributes are available for a proper processing of the actions. * This is called just before a new batch action is created and processing is started. * - * @param batchAction + * @param actionAttributes all attributes for batch action * @return APIMessage if there is an consistency failure */ APIMessage checkConsistency(Map actionAttributes); diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/BatchActionService.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/BatchActionService.java index 52beb2d5..2b02c4f4 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/BatchActionService.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/BatchActionService.java @@ -21,7 +21,7 @@ public interface BatchActionService { /** Validates a given batch action. * - * @param batchAction + * @param batchAction the batch action data * @return Result refer to the BatchAction or to an error when happened */ Result validate(BatchAction batchAction); diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/impl/DeleteExamAction.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/impl/DeleteExamAction.java index 28b2b461..89150aec 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/impl/DeleteExamAction.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/impl/DeleteExamAction.java @@ -13,6 +13,7 @@ import java.util.Collection; import java.util.HashSet; import java.util.Map; +import ch.ethz.seb.sebserver.gbl.api.authorization.PrivilegeType; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.context.annotation.Lazy; @@ -86,7 +87,7 @@ public class DeleteExamAction implements BatchActionExec { @Transactional public Result doSingleAction(final String modelId, final BatchAction batchAction) { return this.examDAO.byModelId(modelId) - .flatMap(this::checkWriteAccess) + .flatMap( exam -> this.checkWriteAccess(exam, batchAction.ownerId)) .flatMap(this::checkNoActiveSEBClientConnections) .flatMap(this::deleteExamDependencies) .flatMap(this::deleteExamWithRefs) @@ -137,7 +138,7 @@ public class DeleteExamAction implements BatchActionExec { } } - private Result checkWriteAccess(final Exam entity) { + private Result checkWriteAccess(final Exam entity, final String ownerId) { if (entity != null) { this.authorization.checkWrite(entity); } diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/impl/ExamConfigDelete.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/impl/ExamConfigDelete.java index 4d213e70..8fc0278b 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/impl/ExamConfigDelete.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/impl/ExamConfigDelete.java @@ -3,7 +3,6 @@ package ch.ethz.seb.sebserver.webservice.servicelayer.bulkaction.impl; import ch.ethz.seb.sebserver.gbl.api.API.BatchActionType; import ch.ethz.seb.sebserver.gbl.api.APIMessage; import ch.ethz.seb.sebserver.gbl.api.EntityType; -import ch.ethz.seb.sebserver.gbl.api.authorization.PrivilegeType; import ch.ethz.seb.sebserver.gbl.model.BatchAction; import ch.ethz.seb.sebserver.gbl.api.APIMessage.APIMessageException; import ch.ethz.seb.sebserver.gbl.model.EntityKey; @@ -21,7 +20,6 @@ import org.springframework.stereotype.Component; import java.util.Arrays; import java.util.HashSet; import java.util.Map; -import java.util.stream.Collectors; @Lazy @Component @@ -58,17 +56,24 @@ public class ExamConfigDelete implements BatchActionExec { public Result doSingleAction(final String modelId, final BatchAction batchAction) { return this.configurationNodeDAO .byModelId(modelId) - .flatMap(examConfig -> this.authorizationService.check(PrivilegeType.MODIFY, examConfig)) - .flatMap(examConfig -> checkDeletionRequirements(examConfig)) + .flatMap(examConfig -> this.checkWriteAccess(examConfig, batchAction.ownerId)) + .flatMap(this::checkDeletionRequirements) .flatMap(examConfig -> this.configurationNodeDAO.delete(new HashSet<>(Arrays.asList(new EntityKey( modelId, EntityType.CONFIGURATION_NODE)))) ) - .map(res -> res.stream().collect(Collectors.toList()).get(0)); + .map(res -> res.stream().toList().get(0)); } - private Result checkDeletionRequirements(ConfigurationNode examConfig){ - Result isNotActive = this.examConfigurationMapDAO.checkNoActiveExamReferences(examConfig.id); + private Result checkWriteAccess(final ConfigurationNode examConfig, final String ownerId) { + if (examConfig != null) { + this.authorizationService.checkWrite(examConfig); + } + return Result.of(examConfig); + } + + private Result checkDeletionRequirements(final ConfigurationNode examConfig){ + final Result isNotActive = this.examConfigurationMapDAO.checkNoActiveExamReferences(examConfig.id); if(!isNotActive.getOrThrow()){ return Result.ofError(new APIMessageException( APIMessage.ErrorMessage.INTEGRITY_VALIDATION diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/impl/ExamConfigStateChange.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/impl/ExamConfigStateChange.java index f96b8dc8..fdadc94d 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/impl/ExamConfigStateChange.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/bulkaction/impl/ExamConfigStateChange.java @@ -83,7 +83,7 @@ public class ExamConfigStateChange implements BatchActionExec { private ConfigurationStatus getTargetState(final Map actionAttributes) { try { - final String targetStateString = actionAttributes.get(BatchAction.ACTION_ATTRIBUT_TARGET_STATE); + final String targetStateString = actionAttributes.get(BatchAction.ACTION_ATTRIBUTE_TARGET_STATE); if (StringUtils.isBlank(targetStateString)) { return null; } diff --git a/src/main/resources/messages.properties b/src/main/resources/messages.properties index 6ff660f5..309a4aed 100644 --- a/src/main/resources/messages.properties +++ b/src/main/resources/messages.properties @@ -245,6 +245,8 @@ sebserver.useraccount.role.EXAM_ADMIN=Exam Administrator sebserver.useraccount.role.EXAM_ADMIN.tooltip=An exam administrator has overall read privileges for the institution but cannot see or manage other user accounts.
An exam administrator is able to create new SEB configurations and import and setup exams. sebserver.useraccount.role.EXAM_SUPPORTER=Exam Supporter sebserver.useraccount.role.EXAM_SUPPORTER.tooltip=An exam supporter can only see and edit the own user account
and monitor exams for that he/she was attached by an exam administrator. +sebserver.useraccount.role.TEACHER=Teacher +sebserver.useraccount.role.TEACHER.tooltip=A teacher can only see and edit the own user account
and monitor exams for that he/she has been imported or was attached by an exam administrator sebserver.useraccount.list.empty=No user account can be found. Please adapt the filter or create a new user account sebserver.useraccount.list.title=User Accounts diff --git a/src/test/java/ch/ethz/seb/sebserver/gui/integration/UseCasesIntegrationTest.java b/src/test/java/ch/ethz/seb/sebserver/gui/integration/UseCasesIntegrationTest.java index e3918447..337ceef4 100644 --- a/src/test/java/ch/ethz/seb/sebserver/gui/integration/UseCasesIntegrationTest.java +++ b/src/test/java/ch/ethz/seb/sebserver/gui/integration/UseCasesIntegrationTest.java @@ -4032,7 +4032,7 @@ public class UseCasesIntegrationTest extends GuiIntegrationTest { .getBuilder(DoBatchAction.class) .withFormParam(Domain.BATCH_ACTION.ATTR_ACTION_TYPE, BatchActionType.EXAM_CONFIG_STATE_CHANGE.name()) .withFormParam(BATCH_ACTION.ATTR_SOURCE_IDS, config.getModelId()) - .withFormParam(BatchAction.ACTION_ATTRIBUT_TARGET_STATE, ConfigurationStatus.CONSTRUCTION.name()) + .withFormParam(BatchAction.ACTION_ATTRIBUTE_TARGET_STATE, ConfigurationStatus.CONSTRUCTION.name()) .call(); assertNotNull(doBatchAction);