From a055e235380422a39661663b1c962c79e368c3d2 Mon Sep 17 00:00:00 2001 From: anhefti Date: Mon, 4 Dec 2023 12:01:19 +0100 Subject: [PATCH] finished up improved SEB client connection handshake --- .../gbl/model/session/ClientConnection.java | 8 ++-- .../monitoring/MonitoringRunningExam.java | 2 +- .../session/ClientConnectionTable.java | 6 +++ .../gui/service/session/ColorData.java | 18 ++------ .../service/session/InstructionProcessor.java | 22 ++++++---- .../dao/impl/ClientConnectionDAOImpl.java | 14 +++--- .../impl/SEBClientConnectionServiceImpl.java | 44 +++++++++++-------- .../weblayer/api/ExamAPI_V1_Controller.java | 4 +- src/main/resources/messages.properties | 1 + .../api/exam/SEBConnectionAPITest.java | 2 +- .../api/exam/SebConnectionTest.java | 17 +++++-- 11 files changed, 80 insertions(+), 58 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 4c93e705..52e6c283 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 @@ -32,7 +32,7 @@ public final class ClientConnection implements GrantEntity { public enum ConnectionStatus { UNDEFINED(0, false, false, false), CONNECTION_REQUESTED(1, true, false, false), - AUTHENTICATED(2, true, true, true), + READY(2, true, true, true), ACTIVE(3, false, true, true), CLOSED(4, false, false, true), DISABLED(5, false, false, false); @@ -59,18 +59,18 @@ public final class ClientConnection implements GrantEntity { public final static List ACTIVE_STATES = Utils.immutableListOf( ConnectionStatus.ACTIVE.name(), - ConnectionStatus.AUTHENTICATED.name(), + ConnectionStatus.READY.name(), ConnectionStatus.CONNECTION_REQUESTED.name()); public final static List SECURE_STATES = Utils.immutableListOf( ConnectionStatus.ACTIVE.name(), - ConnectionStatus.AUTHENTICATED.name(), + ConnectionStatus.READY.name(), ConnectionStatus.CLOSED.name()); public final static List SECURE_CHECK_STATES = Utils.immutableListOf( ConnectionStatus.CONNECTION_REQUESTED.name(), ConnectionStatus.ACTIVE.name(), - ConnectionStatus.AUTHENTICATED.name()); + ConnectionStatus.READY.name()); public static final ClientConnection EMPTY_CLIENT_CONNECTION = new ClientConnection( -1L, -1L, -1L, diff --git a/src/main/java/ch/ethz/seb/sebserver/gui/content/monitoring/MonitoringRunningExam.java b/src/main/java/ch/ethz/seb/sebserver/gui/content/monitoring/MonitoringRunningExam.java index 8d8dc998..3062571a 100644 --- a/src/main/java/ch/ethz/seb/sebserver/gui/content/monitoring/MonitoringRunningExam.java +++ b/src/main/java/ch/ethz/seb/sebserver/gui/content/monitoring/MonitoringRunningExam.java @@ -546,7 +546,7 @@ public class MonitoringRunningExam implements TemplateComposer { } /** This holds the filter action items and implements the specific GUI update for it */ - private class FilterGUIUpdate implements FullPageMonitoringGUIUpdate { + private static class FilterGUIUpdate implements FullPageMonitoringGUIUpdate { private final PolyglotPageService polyglotPageService; private final TreeItem[] actionItemPerStateFilter = new TreeItem[ConnectionStatus.values().length]; diff --git a/src/main/java/ch/ethz/seb/sebserver/gui/service/session/ClientConnectionTable.java b/src/main/java/ch/ethz/seb/sebserver/gui/service/session/ClientConnectionTable.java index c33793d3..bfc5dd04 100644 --- a/src/main/java/ch/ethz/seb/sebserver/gui/service/session/ClientConnectionTable.java +++ b/src/main/java/ch/ethz/seb/sebserver/gui/service/session/ClientConnectionTable.java @@ -629,6 +629,12 @@ public final class ClientConnectionTable implements FullPageMonitoringGUIUpdate .thenComparingInt(UpdatableTableItem::thresholdsWeight) .thenComparing(UpdatableTableItem::getConnectionIdentifier) .compare(this, other); + +// return Comparator.comparing(UpdatableTableItem::getConnectionIdentifier) +// .thenComparingInt(UpdatableTableItem::thresholdsWeight) +// .thenComparingInt(UpdatableTableItem::notificationWeight) +// .thenComparingInt(UpdatableTableItem::statusWeight) +// .compare(this, other); } @Override diff --git a/src/main/java/ch/ethz/seb/sebserver/gui/service/session/ColorData.java b/src/main/java/ch/ethz/seb/sebserver/gui/service/session/ColorData.java index dd857e6f..f9785db3 100644 --- a/src/main/java/ch/ethz/seb/sebserver/gui/service/session/ColorData.java +++ b/src/main/java/ch/ethz/seb/sebserver/gui/service/session/ColorData.java @@ -42,7 +42,7 @@ public class ColorData { switch (status) { case CONNECTION_REQUESTED: - case AUTHENTICATED: + case READY: case ACTIVE: { if (entry.grantDenied()) { return this.color3; @@ -62,23 +62,11 @@ public class ColorData { } int statusWeight(final MonitoringEntry entry) { - if (entry == null) { + if (entry == null || entry.getStatus() == null) { return 100; } - switch (entry.getStatus()) { - case CONNECTION_REQUESTED: - case AUTHENTICATED: { - return 0; - } - case ACTIVE: { - return 2; - } - case CLOSED: - return 3; - default: - return 5; - } + return entry.getStatus().code; } } diff --git a/src/main/java/ch/ethz/seb/sebserver/gui/service/session/InstructionProcessor.java b/src/main/java/ch/ethz/seb/sebserver/gui/service/session/InstructionProcessor.java index 3e54e549..c8746b66 100644 --- a/src/main/java/ch/ethz/seb/sebserver/gui/service/session/InstructionProcessor.java +++ b/src/main/java/ch/ethz/seb/sebserver/gui/service/session/InstructionProcessor.java @@ -48,6 +48,14 @@ public class InstructionProcessor { private static final Logger log = LoggerFactory.getLogger(InstructionProcessor.class); + private static final Predicate ALL_BUT_DISABLED_STATES = + ClientMonitoringDataView.getStatusPredicate( + ConnectionStatus.CONNECTION_REQUESTED, + ConnectionStatus.UNDEFINED, + ConnectionStatus.CLOSED, + ConnectionStatus.ACTIVE, + ConnectionStatus.READY); + private final RestService restService; private final JSONMapper jsonMapper; @@ -77,6 +85,7 @@ public class InstructionProcessor { final Set connectionTokens = selectionFunction .apply(ClientMonitoringDataView.getStatusPredicate( ConnectionStatus.CONNECTION_REQUESTED, + ConnectionStatus.READY, ConnectionStatus.ACTIVE)); if (connectionTokens.isEmpty()) { @@ -97,7 +106,7 @@ public class InstructionProcessor { null); processInstruction(() -> this.restService.getBuilder(PropagateInstruction.class) - .withURIVariable(API.PARAM_PARENT_MODEL_ID, String.valueOf(examId)) + .withURIVariable(API.PARAM_PARENT_MODEL_ID, examId) .withBody(clientInstruction) .call() .getOrThrow(), @@ -154,19 +163,14 @@ public class InstructionProcessor { } } + + public void disableConnection( final Long examId, final Function, Set> selectionFunction, final PageContext pageContext) { - final Set connectionTokens = selectionFunction - .apply(ClientMonitoringDataView.getStatusPredicate( - ConnectionStatus.CONNECTION_REQUESTED, - ConnectionStatus.UNDEFINED, - ConnectionStatus.CLOSED, - ConnectionStatus.ACTIVE, - ConnectionStatus.AUTHENTICATED)); - + final Set connectionTokens = selectionFunction.apply(ALL_BUT_DISABLED_STATES); if (connectionTokens.isEmpty()) { return; } diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/dao/impl/ClientConnectionDAOImpl.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/dao/impl/ClientConnectionDAOImpl.java index 9ae6f7ea..d9a76542 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/dao/impl/ClientConnectionDAOImpl.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/dao/impl/ClientConnectionDAOImpl.java @@ -286,7 +286,7 @@ public class ClientConnectionDAOImpl implements ClientConnectionDAO { .and(ClientConnectionRecordDynamicSqlSupport.status, isNotEqualTo(ConnectionStatus.ACTIVE.name())) .and( ClientConnectionRecordDynamicSqlSupport.status, - isNotEqualTo(ConnectionStatus.AUTHENTICATED.name())) + isNotEqualTo(ConnectionStatus.READY.name())) .and(ClientConnectionRecordDynamicSqlSupport.status, isNotEqualTo(ConnectionStatus.CONNECTION_REQUESTED.name())) .build() @@ -777,10 +777,14 @@ public class ClientConnectionDAOImpl implements ClientConnectionDAO { SqlBuilder.isEqualTo(examId)) .and( ClientConnectionRecordDynamicSqlSupport.status, - SqlBuilder.isEqualTo(ConnectionStatus.ACTIVE.name()), - SqlBuilder.or( - ClientConnectionRecordDynamicSqlSupport.status, - SqlBuilder.isEqualTo(ConnectionStatus.CONNECTION_REQUESTED.name()))) + SqlBuilder.isIn(ClientConnection.ACTIVE_STATES) + ) +// .and( +// ClientConnectionRecordDynamicSqlSupport.status, +// SqlBuilder.isEqualTo(ConnectionStatus.ACTIVE.name()), +// SqlBuilder.or( +// ClientConnectionRecordDynamicSqlSupport.status, +// SqlBuilder.isEqualTo(ConnectionStatus.CONNECTION_REQUESTED.name()))) .build() .execute() .stream() 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 eee9e62e..40ca0b95 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 @@ -25,7 +25,6 @@ import ch.ethz.seb.sebserver.gbl.util.Utils; import ch.ethz.seb.sebserver.webservice.servicelayer.session.SEBClientInstructionService; import org.apache.commons.lang3.BooleanUtils; import org.apache.commons.lang3.StringUtils; -import org.apache.tomcat.jni.Address; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.context.annotation.Lazy; @@ -64,7 +63,7 @@ public class SEBClientConnectionServiceImpl implements SEBClientConnectionServic .getStatusPredicate( ConnectionStatus.UNDEFINED, ConnectionStatus.CONNECTION_REQUESTED, - ConnectionStatus.AUTHENTICATED, + ConnectionStatus.READY, ConnectionStatus.ACTIVE, ConnectionStatus.CLOSED); @@ -266,8 +265,8 @@ public class SEBClientConnectionServiceImpl implements SEBClientConnectionServic clientConnection.id, null, examId, - (userSessionId != null && currentStatus == ConnectionStatus.CONNECTION_REQUESTED) - ? ConnectionStatus.AUTHENTICATED + (StringUtils.isNotBlank(userSessionId) && currentStatus == ConnectionStatus.READY) + ? ConnectionStatus.ACTIVE : null, null, updateUserSessionId, @@ -345,7 +344,6 @@ public class SEBClientConnectionServiceImpl implements SEBClientConnectionServic connectionToken, institutionId, examId, clientAddress, userSessionId, clientId); } - final String updateUserSessionId = updateUserSessionId( _examId, userSessionId, @@ -364,12 +362,16 @@ public class SEBClientConnectionServiceImpl implements SEBClientConnectionServic ? clientId : clientConnection.sebClientUserId; + + // create new ClientConnection for update final ClientConnection establishedClientConnection = new ClientConnection( clientConnection.id, null, currentExamId, - ConnectionStatus.ACTIVE, + StringUtils.isNotBlank(userSessionId) || alreadyAuthenticated(clientConnection) + ? ConnectionStatus.ACTIVE + : ConnectionStatus.READY, null, updateUserSessionId, StringUtils.isNotBlank(clientAddress) ? clientAddress : null, @@ -390,20 +392,16 @@ public class SEBClientConnectionServiceImpl implements SEBClientConnectionServic null); // ClientConnection integrity check - // institutionId, connectionToken and clientAddress must be set - // The status ins not already active - // and if this is not a VDI prime connection, the examId must also be set if (clientConnection.institutionId == null || clientConnection.connectionToken == null || - establishedClientConnection.examId == null || - clientConnection.clientAddress == null || - establishedClientConnection.status != ConnectionStatus.ACTIVE || - (!BooleanUtils.isTrue(clientConnection.vdi) && currentExamId == null)) { + currentExamId == null || + (clientConnection.clientAddress == null && clientAddress == null)) { - log.error("ClientConnection integrity violation, clientConnection: {}, establishedClientConnection: {}", + log.error("ClientConnection integrity violation, clientConnection: {}, updatedClientConnection: {}", clientConnection, establishedClientConnection); - throw new IllegalStateException("ClientConnection integrity violation"); + + throw new APIConstraintViolationException("ClientConnection integrity violation"); } final ClientConnection updatedClientConnection = this.clientConnectionDAO @@ -635,7 +633,10 @@ public class SEBClientConnectionServiceImpl implements SEBClientConnectionServic } } - private void writeSEBClientErrors(HttpServletResponse response, Collection errorMessages) { + private void writeSEBClientErrors( + final HttpServletResponse response, + final Collection errorMessages) { + try { response.getOutputStream().write(Utils.toByteArray(this.jsonMapper.writeValueAsString(errorMessages))); } catch (final Exception e1) { @@ -663,7 +664,7 @@ public class SEBClientConnectionServiceImpl implements SEBClientConnectionServic false ); - throw new IllegalArgumentException( + throw new APIConstraintViolationException( "ClientConnection integrity violation: client connection is not in expected state"); } @@ -842,7 +843,7 @@ public class SEBClientConnectionServiceImpl implements SEBClientConnectionServic if (currentExamId != null && !examId.equals(currentExamId)) { log.error("Exam integrity violation: another examId is already set for the connection: {}", ccToken); - throw new IllegalArgumentException( + throw new APIConstraintViolationException( "Exam integrity violation: another examId is already set for the connection"); } @@ -914,4 +915,11 @@ public class SEBClientConnectionServiceImpl implements SEBClientConnectionServic .onError(error -> log.error("Failed to get hash signature from sent app signature key: ", error)) .getOr(null); } + + private boolean alreadyAuthenticated(final ClientConnection clientConnection) { + return clientConnection.userSessionId != null && + !clientConnection.userSessionId.equals(clientConnection.clientAddress) && + !clientConnection.userSessionId.equals(clientConnection.sebClientUserId) && + !clientConnection.userSessionId.equals(clientConnection.sebMachineName); + } } 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 408530ab..193f4492 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 @@ -237,7 +237,7 @@ public class ExamAPI_V1_Controller { @RequestParam(name = API.EXAM_API_USER_SESSION_ID, required = false) final String userSessionId, @RequestParam(name = API.EXAM_API_PARAM_SEB_VERSION, required = false) final String sebVersion, @RequestParam(name = API.EXAM_API_PARAM_SEB_OS_NAME, required = false) final String sebOSName, - @RequestParam(name = API.EXAM_API_PARAM_SEB_MACHINE_NAME, required = false) final String sebMachinName, + @RequestParam(name = API.EXAM_API_PARAM_SEB_MACHINE_NAME, required = false) final String setMachineName, @RequestParam( name = API.EXAM_API_PARAM_SIGNATURE_KEY, required = false) final String browserSignatureKey, @@ -260,7 +260,7 @@ public class ExamAPI_V1_Controller { remoteAddr, sebVersion, sebOSName, - sebMachinName, + setMachineName, userSessionId, clientId, browserSignatureKey) diff --git a/src/main/resources/messages.properties b/src/main/resources/messages.properties index 0ab58a0a..ab231750 100644 --- a/src/main/resources/messages.properties +++ b/src/main/resources/messages.properties @@ -2320,6 +2320,7 @@ sebserver.monitoring.exam.connection.notificationlist.type=Notification Type sebserver.monitoring.exam.connection.status.UNDEFINED=Undefined sebserver.monitoring.exam.connection.status.CONNECTION_REQUESTED=Connecting sebserver.monitoring.exam.connection.status.AUTHENTICATED=Authenticated +sebserver.monitoring.exam.connection.status.READY=Ready sebserver.monitoring.exam.connection.status.ACTIVE=Active sebserver.monitoring.exam.connection.status.CLOSED=Closed sebserver.monitoring.exam.connection.status.ABORTED=Aborted diff --git a/src/test/java/ch/ethz/seb/sebserver/webservice/integration/api/exam/SEBConnectionAPITest.java b/src/test/java/ch/ethz/seb/sebserver/webservice/integration/api/exam/SEBConnectionAPITest.java index 20fecdc9..b0c0ad38 100644 --- a/src/test/java/ch/ethz/seb/sebserver/webservice/integration/api/exam/SEBConnectionAPITest.java +++ b/src/test/java/ch/ethz/seb/sebserver/webservice/integration/api/exam/SEBConnectionAPITest.java @@ -162,7 +162,7 @@ public class SEBConnectionAPITest extends ExamAPIIntegrationTester { .byConnectionToken(connectionToken) .getOrThrow(); assertNotNull(clientConnection); - assertEquals("AUTHENTICATED", clientConnection.getStatus().name()); + assertEquals("CONNECTION_REQUESTED", clientConnection.getStatus().name()); assertEquals(connectionToken, clientConnection.connectionToken); assertEquals(1, (long) clientConnection.institutionId); assertEquals(2, (long) clientConnection.examId); 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 a55a2e61..9d87d07b 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 @@ -238,7 +238,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("AUTHENTICATED", String.valueOf(clientConnectionRecord.getStatus())); + assertEquals("CONNECTION_REQUESTED", String.valueOf(clientConnectionRecord.getStatus())); assertNotNull(clientConnectionRecord.getConnectionToken()); assertNotNull(clientConnectionRecord.getClientAddress()); assertEquals("-- (userSessionId)", clientConnectionRecord.getExamUserSessionId()); @@ -361,14 +361,14 @@ public class SebConnectionTest extends ExamAPIIntegrationTester { null); // check correct response - assertTrue(HttpStatus.OK.value() != updatedConnection.getStatus()); + assertTrue(HttpStatus.BAD_REQUEST.value() == updatedConnection.getStatus()); final String contentAsString = updatedConnection.getContentAsString(); final Collection errorMessage = this.jsonMapper.readValue( contentAsString, new TypeReference>() { }); final APIMessage error = errorMessage.iterator().next(); - assertEquals(ErrorMessage.UNEXPECTED.messageCode, error.messageCode); + assertEquals(ErrorMessage.ILLEGAL_API_ARGUMENT.messageCode, error.messageCode); assertEquals("ClientConnection integrity violation", error.details); // check correct stored (no changes) @@ -394,6 +394,17 @@ public class SebConnectionTest extends ExamAPIIntegrationTester { assertNotNull(ccdi); assertNull(ccdi.clientConnection.examId); assertTrue(ccdi.indicatorValues.isEmpty()); + + // check update to active + MockHttpServletResponse updateResponse = super.updateConnection( + accessToken, + connectionToken, + null, + "test-user"); + + + + } @Test