From edb751de87c7309957bfd9f3d51e7b3b68210668 Mon Sep 17 00:00:00 2001 From: anhefti Date: Mon, 2 Sep 2019 10:20:51 +0200 Subject: [PATCH] refactored client connection (more integrity checks) --- .../gbl/model/session/ClientConnection.java | 4 +- .../servicelayer/batch/BatchConfig.java | 45 +++--- .../session/ExamSessionService.java | 6 + .../session/SebClientConnectionService.java | 4 + .../session/impl/ExamSessionServiceImpl.java | 7 +- .../impl/SebClientConnectionServiceImpl.java | 128 ++++++++++++------ .../weblayer/api/ExamAPI_V1_Controller.java | 54 +++----- .../api/exam/SebConnectionTest.java | 2 +- 8 files changed, 143 insertions(+), 107 deletions(-) diff --git a/src/main/java/ch/ethz/seb/sebserver/gbl/model/session/ClientConnection.java b/src/main/java/ch/ethz/seb/sebserver/gbl/model/session/ClientConnection.java index 11479509..31f57254 100644 --- a/src/main/java/ch/ethz/seb/sebserver/gbl/model/session/ClientConnection.java +++ b/src/main/java/ch/ethz/seb/sebserver/gbl/model/session/ClientConnection.java @@ -81,7 +81,7 @@ public final class ClientConnection implements GrantEntity { @JsonProperty(Domain.CLIENT_CONNECTION.ATTR_EXAM_USER_SESSION_IDENTIFER) final String userSessionId, @JsonProperty(Domain.CLIENT_CONNECTION.ATTR_CLIENT_ADDRESS) final String clientAddress, @JsonProperty(Domain.CLIENT_CONNECTION.ATTR_VIRTUAL_CLIENT_ADDRESS) final String virtualClientAddress, - @JsonProperty(Domain.CLIENT_CONNECTION.ATTR_CREATION_TIME) final Long creationTim) { + @JsonProperty(Domain.CLIENT_CONNECTION.ATTR_CREATION_TIME) final Long creationTime) { this.id = id; this.institutionId = institutionId; @@ -91,7 +91,7 @@ public final class ClientConnection implements GrantEntity { this.userSessionId = userSessionId; this.clientAddress = clientAddress; this.virtualClientAddress = virtualClientAddress; - this.creationTime = creationTim; + this.creationTime = creationTime; } @Override diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/batch/BatchConfig.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/batch/BatchConfig.java index 1e5cb5ba..25bfa2ee 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/batch/BatchConfig.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/batch/BatchConfig.java @@ -8,36 +8,29 @@ package ch.ethz.seb.sebserver.webservice.servicelayer.batch; -import org.quartz.CronScheduleBuilder; -import org.quartz.JobBuilder; -import org.quartz.JobDetail; -import org.quartz.Trigger; -import org.quartz.TriggerBuilder; -import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @Configuration public class BatchConfig { - @Bean - public JobDetail jobADetails() { - return JobBuilder - .newJob(SimpleBatchJob.class) - .withIdentity("sampleJobA") - .build(); - } - - @Bean - public Trigger jobATrigger(final JobDetail jobADetails) { - - return TriggerBuilder - .newTrigger() - .forJob(jobADetails) - .withIdentity("sampleTriggerA") - - .withSchedule(CronScheduleBuilder.cronSchedule("0/30 0 0 ? * * *")) - .startNow() - .build(); - } +// @Bean +// public JobDetail jobADetails() { +// return JobBuilder +// .newJob(SimpleBatchJob.class) +// .withIdentity("sampleJobA") +// .build(); +// } +// +// @Bean +// public Trigger jobATrigger(final JobDetail jobADetails) { +// +// return TriggerBuilder +// .newTrigger() +// .forJob(jobADetails) +// .withIdentity("sampleTriggerA") +// +// .withSchedule(CronScheduleBuilder.cronSchedule("0/30 0 0 ? * * *")) +// .build(); +// } } diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/session/ExamSessionService.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/session/ExamSessionService.java index d2ac5f09..3fedaf3c 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/session/ExamSessionService.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/session/ExamSessionService.java @@ -15,11 +15,17 @@ import java.util.function.Predicate; import ch.ethz.seb.sebserver.gbl.model.exam.Exam; import ch.ethz.seb.sebserver.gbl.model.session.ClientConnectionData; import ch.ethz.seb.sebserver.gbl.util.Result; +import ch.ethz.seb.sebserver.webservice.servicelayer.dao.ExamDAO; import ch.ethz.seb.sebserver.webservice.servicelayer.dao.FilterMap; /** A Service to handle running exam sessions */ public interface ExamSessionService { + /** Get the underling ExamDAO service. + * + * @return the underling ExamDAO service. */ + ExamDAO getExamDAO(); + /** Indicates whether an Exam is currently running or not. * * @param examId the PK of the Exam to test diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/session/SebClientConnectionService.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/session/SebClientConnectionService.java index 29daf3e7..c75ae958 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/session/SebClientConnectionService.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/session/SebClientConnectionService.java @@ -8,6 +8,8 @@ package ch.ethz.seb.sebserver.webservice.servicelayer.session; +import java.security.Principal; + import ch.ethz.seb.sebserver.gbl.model.session.ClientConnection; import ch.ethz.seb.sebserver.gbl.model.session.ClientEvent; import ch.ethz.seb.sebserver.gbl.util.Result; @@ -25,12 +27,14 @@ public interface SebClientConnectionService { * A connection-token to identify the connection is generated and stored within the * returned ClientConnection. * + * @param principal the client connection Principal from REST controller interface * @param institutionId The institution identifier * @param clientAddress The clients remote IP address * @param examId the exam identifier (can be null) * @return A Result refer to the newly created ClientConnection in state: CONNECTION_REQUESTED, or refer to an error * if happened */ Result createClientConnection( + Principal principal, Long institutionId, String clientAddress, Long examId); diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/session/impl/ExamSessionServiceImpl.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/session/impl/ExamSessionServiceImpl.java index 0a124dfb..4c8ab632 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/session/impl/ExamSessionServiceImpl.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/session/impl/ExamSessionServiceImpl.java @@ -58,6 +58,11 @@ public class ExamSessionServiceImpl implements ExamSessionService { this.cacheManager = cacheManager; } + @Override + public ExamDAO getExamDAO() { + return this.examDAO; + } + @Override public boolean isExamRunning(final Long examId) { return !getRunningExam(examId).hasError(); @@ -199,7 +204,7 @@ public class ExamSessionServiceImpl implements ExamSessionService { // evict also cached ping record this.examSessionCacheService.evictPingRecord(token); }); - } catch (Exception e) { + } catch (final Exception e) { log.error("Unexpected error while trying to flush cache for exam: ", exam, e); } } diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/session/impl/SebClientConnectionServiceImpl.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/session/impl/SebClientConnectionServiceImpl.java index 5a678ea8..a1a5f66c 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/session/impl/SebClientConnectionServiceImpl.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/session/impl/SebClientConnectionServiceImpl.java @@ -8,6 +8,7 @@ package ch.ethz.seb.sebserver.webservice.servicelayer.session.impl; +import java.security.Principal; import java.util.UUID; import org.slf4j.Logger; @@ -23,10 +24,12 @@ import ch.ethz.seb.sebserver.gbl.profile.WebServiceProfile; import ch.ethz.seb.sebserver.gbl.util.Result; import ch.ethz.seb.sebserver.gbl.util.Utils; import ch.ethz.seb.sebserver.webservice.servicelayer.dao.ClientConnectionDAO; +import ch.ethz.seb.sebserver.webservice.servicelayer.dao.SebClientConfigDAO; import ch.ethz.seb.sebserver.webservice.servicelayer.session.EventHandlingStrategy; import ch.ethz.seb.sebserver.webservice.servicelayer.session.ExamSessionService; import ch.ethz.seb.sebserver.webservice.servicelayer.session.PingHandlingStrategy; import ch.ethz.seb.sebserver.webservice.servicelayer.session.SebClientConnectionService; +import ch.ethz.seb.sebserver.webservice.weblayer.api.APIConstraintViolationException; @Lazy @Service @@ -40,29 +43,51 @@ public class SebClientConnectionServiceImpl implements SebClientConnectionServic private final EventHandlingStrategy eventHandlingStrategy; private final ClientConnectionDAO clientConnectionDAO; private final PingHandlingStrategy pingHandlingStrategy; + private final SebClientConfigDAO sebClientConfigDAO; protected SebClientConnectionServiceImpl( final ExamSessionService examSessionService, final ExamSessionCacheService examSessionCacheService, final ClientConnectionDAO clientConnectionDAO, final EventHandlingStrategyFactory eventHandlingStrategyFactory, - final PingHandlingStrategyFactory pingHandlingStrategyFactory) { + final PingHandlingStrategyFactory pingHandlingStrategyFactory, + final SebClientConfigDAO sebClientConfigDAO) { this.examSessionService = examSessionService; this.examSessionCacheService = examSessionCacheService; this.clientConnectionDAO = clientConnectionDAO; this.pingHandlingStrategy = pingHandlingStrategyFactory.get(); this.eventHandlingStrategy = eventHandlingStrategyFactory.get(); + this.sebClientConfigDAO = sebClientConfigDAO; } @Override public Result createClientConnection( + final Principal principal, final Long institutionId, final String clientAddress, final Long examId) { return Result.tryCatch(() -> { + final Long clientsInstitution = getInstitutionId(principal); + if (!clientsInstitution.equals(institutionId)) { + log.error("Institutional integrity violation: requested institution: {} authenticated institution: {}", + institutionId, + clientsInstitution); + throw new APIConstraintViolationException("Institutional integrity violation"); + } + + if (log.isDebugEnabled()) { + log.debug("Request received on Exam Client Connection create endpoint: " + + "institution: {} " + + "exam: {} " + + "client-address: {}", + institutionId, + examId, + clientAddress); + } + if (log.isDebugEnabled()) { log.debug("SEB client connection attempt, create ClientConnection for " + "instituion {} " @@ -131,21 +156,16 @@ public class SebClientConnectionServiceImpl implements SebClientConnectionServic final ClientConnection clientConnection = getClientConnection(connectionToken); - checkInstitutionalIntegrity( - institutionId, - clientConnection); + checkInstitutionalIntegrity(institutionId, clientConnection); + checkExamIntegrity(examId, clientConnection); - // examId integrity check - if (examId != null && - clientConnection.examId != null && - !examId.equals(clientConnection.examId)) { - - log.error("Exam integrity violation: another examId is already set for the connection: {}", + // connection integrity check + if (clientConnection.status != ConnectionStatus.CONNECTION_REQUESTED) { + log.error("ClientConnection integrity violation: client connection is not in expected state: {}", clientConnection); throw new IllegalArgumentException( - "Exam integrity violation: another examId is already set for the connection"); + "ClientConnection integrity violation: client connection is not in expected state"); } - checkExamRunning(examId); // userSessionId integrity check if (userSessionId != null && @@ -169,7 +189,7 @@ public class SebClientConnectionServiceImpl implements SebClientConnectionServic clientConnection.id, null, examId, - null, + (userSessionId != null) ? ConnectionStatus.AUTHENTICATED : null, null, userSessionId, null, @@ -219,20 +239,21 @@ public class SebClientConnectionServiceImpl implements SebClientConnectionServic userSessionId); } - checkExamRunning(examId); - final ClientConnection clientConnection = getClientConnection(connectionToken); + checkInstitutionalIntegrity(institutionId, clientConnection); + checkExamIntegrity(examId, clientConnection); - checkInstitutionalIntegrity( - institutionId, - clientConnection); - - // Exam integrity - if (clientConnection.examId != null && examId != null && !examId.equals(clientConnection.examId)) { - log.error("Exam integrity violation with examId: {} on clientConnection: {}", - examId, + // connection integrity check + if (clientConnection.status == ConnectionStatus.CONNECTION_REQUESTED) { + // TODO discuss if we need a flag on exam domain level that indicates whether unauthenticated connection + // are allowed or not + log.warn("ClientConnection integrity warning: client connection is not authenticated: {}", clientConnection); - throw new IllegalAccessError("Exam integrity violation"); + } else if (clientConnection.status != ConnectionStatus.AUTHENTICATED) { + log.error("ClientConnection integrity violation: client connection is not in expected state: {}", + clientConnection); + throw new IllegalArgumentException( + "ClientConnection integrity violation: client connection is not in expected state"); } final String virtualClientAddress = getVirtualClientAddress( @@ -240,6 +261,7 @@ public class SebClientConnectionServiceImpl implements SebClientConnectionServic clientAddress, clientConnection.clientAddress); + // create new ClientConnection for update final ClientConnection establishedClientConnection = new ClientConnection( clientConnection.id, null, @@ -312,20 +334,26 @@ public class SebClientConnectionServiceImpl implements SebClientConnectionServic .byConnectionToken(connectionToken) .getOrThrow(); - final ClientConnection updatedClientConnection = this.clientConnectionDAO.save(new ClientConnection( - clientConnection.id, - null, - null, - ConnectionStatus.CLOSED, - null, - null, - null, - null, - null)).getOrThrow(); + ClientConnection updatedClientConnection; + if (clientConnection.status != ConnectionStatus.CLOSED) { + updatedClientConnection = this.clientConnectionDAO.save(new ClientConnection( + clientConnection.id, + null, + null, + ConnectionStatus.CLOSED, + null, + null, + null, + null, + null)).getOrThrow(); - if (log.isDebugEnabled()) { - log.debug("SEB client connection: successfully closed ClientConnection: {}", - clientConnection); + if (log.isDebugEnabled()) { + log.debug("SEB client connection: successfully closed ClientConnection: {}", + clientConnection); + } + } else { + log.warn("SEB client connection is already closed: {}", clientConnection); + updatedClientConnection = clientConnection; } // evict cached ClientConnection @@ -390,9 +418,10 @@ public class SebClientConnectionServiceImpl implements SebClientConnectionServic return clientConnection; } - private void checkInstitutionalIntegrity(final Long institutionId, final ClientConnection clientConnection) - throws IllegalAccessError { - // Institutional integrity + private void checkInstitutionalIntegrity( + final Long institutionId, + final ClientConnection clientConnection) throws IllegalAccessError { + if (!institutionId.equals(clientConnection.institutionId)) { log.error("Instituion integrity violation with institution: {} on clientConnection: {}", institutionId, @@ -437,4 +466,23 @@ public class SebClientConnectionServiceImpl implements SebClientConnectionServic .getType() == ExamType.VDI; } + private Long getInstitutionId(final Principal principal) { + final String clientId = principal.getName(); + return this.sebClientConfigDAO.byClientName(clientId) + .getOrThrow().institutionId; + } + + private void checkExamIntegrity(final Long examId, final ClientConnection clientConnection) { + if (examId != null && + clientConnection.examId != null && + !examId.equals(clientConnection.examId)) { + + log.error("Exam integrity violation: another examId is already set for the connection: {}", + clientConnection); + throw new IllegalArgumentException( + "Exam integrity violation: another examId is already set for the connection"); + } + checkExamRunning(examId); + } + } diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/weblayer/api/ExamAPI_V1_Controller.java b/src/main/java/ch/ethz/seb/sebserver/webservice/weblayer/api/ExamAPI_V1_Controller.java index a8a4f077..93c2fb28 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/weblayer/api/ExamAPI_V1_Controller.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/weblayer/api/ExamAPI_V1_Controller.java @@ -43,7 +43,6 @@ import ch.ethz.seb.sebserver.gbl.model.session.PingResponse; import ch.ethz.seb.sebserver.gbl.model.session.RunningExamInfo; import ch.ethz.seb.sebserver.gbl.profile.WebServiceProfile; import ch.ethz.seb.sebserver.gbl.util.Utils; -import ch.ethz.seb.sebserver.webservice.servicelayer.dao.ExamDAO; import ch.ethz.seb.sebserver.webservice.servicelayer.dao.LmsSetupDAO; import ch.ethz.seb.sebserver.webservice.servicelayer.dao.SebClientConfigDAO; import ch.ethz.seb.sebserver.webservice.servicelayer.session.ExamSessionService; @@ -56,7 +55,6 @@ public class ExamAPI_V1_Controller { private static final Logger log = LoggerFactory.getLogger(ExamAPI_V1_Controller.class); - private final ExamDAO examDAO; private final LmsSetupDAO lmsSetupDAO; private final ExamSessionService examSessionService; private final SebClientConnectionService sebClientConnectionService; @@ -64,14 +62,12 @@ public class ExamAPI_V1_Controller { private final JSONMapper jsonMapper; protected ExamAPI_V1_Controller( - final ExamDAO examDAO, final LmsSetupDAO lmsSetupDAO, final ExamSessionService examSessionService, final SebClientConnectionService sebClientConnectionService, final SebClientConfigDAO sebClientConfigDAO, final JSONMapper jsonMapper) { - this.examDAO = examDAO; this.lmsSetupDAO = lmsSetupDAO; this.examSessionService = examSessionService; this.sebClientConnectionService = sebClientConnectionService; @@ -101,25 +97,17 @@ public class ExamAPI_V1_Controller { final Long examId = (examIdRequestParam != null) ? examIdRequestParam : mapper.getLong(API.EXAM_API_PARAM_EXAM_ID); - final Long clientsInstitution = getInstitutionId(principal); - if (!clientsInstitution.equals(institutionId)) { - log.error("Institutional integrity violation: requested institution: {} authenticated institution: {}", - institutionId, - clientsInstitution); - throw new APIConstraintViolationException("Institutional integrity violation"); - } + // Create and get new ClientConnection if all integrity checks passes + final ClientConnection clientConnection = this.sebClientConnectionService + .createClientConnection(principal, institutionId, remoteAddr, examId) + .getOrThrow(); - if (log.isDebugEnabled()) { - log.debug("Request received on Exam Client Connection create endpoint: " - + "institution: {} " - + "exam: {} " - + "client-address: {}", - institutionId, - examId, - remoteAddr); - } + response.setHeader( + API.EXAM_API_SEB_CONNECTION_TOKEN, + clientConnection.connectionToken); + // Crate list of running exams List result; if (examId == null) { result = this.examSessionService.getRunningExamsForInstitution(institutionId) @@ -128,7 +116,7 @@ public class ExamAPI_V1_Controller { .map(this::createRunningExamInfo) .collect(Collectors.toList()); } else { - final Exam exam = this.examDAO.byPK(examId) + final Exam exam = this.examSessionService.getExamDAO().byPK(examId) .getOrThrow(); result = Arrays.asList(createRunningExamInfo(exam)); @@ -139,25 +127,9 @@ public class ExamAPI_V1_Controller { throw new IllegalStateException("There are no currently running exams"); } - final ClientConnection clientConnection = this.sebClientConnectionService - .createClientConnection(institutionId, remoteAddr, examId) - .getOrThrow(); - - response.setHeader( - API.EXAM_API_SEB_CONNECTION_TOKEN, - clientConnection.connectionToken); - return result; } - private RunningExamInfo createRunningExamInfo(final Exam exam) { - return new RunningExamInfo( - exam, - this.lmsSetupDAO.byPK(exam.lmsSetupId) - .map(lms -> lms.lmsType) - .getOr(null)); - } - @RequestMapping( path = API.EXAM_API_HANDSHAKE_ENDPOINT, method = RequestMethod.PATCH, @@ -352,4 +324,12 @@ public class ExamAPI_V1_Controller { .getOrThrow().institutionId; } + private RunningExamInfo createRunningExamInfo(final Exam exam) { + return new RunningExamInfo( + exam, + this.lmsSetupDAO.byPK(exam.lmsSetupId) + .map(lms -> lms.lmsType) + .getOr(null)); + } + } diff --git a/src/test/java/ch/ethz/seb/sebserver/webservice/integration/api/exam/SebConnectionTest.java b/src/test/java/ch/ethz/seb/sebserver/webservice/integration/api/exam/SebConnectionTest.java index d4035045..a3f595df 100644 --- a/src/test/java/ch/ethz/seb/sebserver/webservice/integration/api/exam/SebConnectionTest.java +++ b/src/test/java/ch/ethz/seb/sebserver/webservice/integration/api/exam/SebConnectionTest.java @@ -216,7 +216,7 @@ public class SebConnectionTest extends ExamAPIIntegrationTester { final ClientConnectionRecord clientConnectionRecord = records.get(0); assertEquals("1", String.valueOf(clientConnectionRecord.getInstitutionId())); assertEquals("2", String.valueOf(clientConnectionRecord.getExamId())); - assertEquals("CONNECTION_REQUESTED", String.valueOf(clientConnectionRecord.getStatus())); + assertEquals("AUTHENTICATED", String.valueOf(clientConnectionRecord.getStatus())); assertNotNull(clientConnectionRecord.getConnectionToken()); assertNotNull(clientConnectionRecord.getClientAddress()); assertEquals("userSessionId", clientConnectionRecord.getExamUserSessionIdentifer());