From 33bd6da1d27c8318499405f9931e130416ec5dab Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Tue, 9 Jun 2026 16:19:05 +0100 Subject: [PATCH 01/31] initial commit with empty endpoint --- .../dtg/isaac/api/SkillsQuestionFacade.java | 46 +++++++++++++++++++ .../dtg/isaac/api/SkillsQuestionFacadeIT.java | 30 ++++++++++++ 2 files changed, 76 insertions(+) create mode 100644 src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsQuestionFacade.java create mode 100644 src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsQuestionFacadeIT.java diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsQuestionFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsQuestionFacade.java new file mode 100644 index 0000000000..cd447bd4bf --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsQuestionFacade.java @@ -0,0 +1,46 @@ +package uk.ac.cam.cl.dtg.isaac.api; + +import com.google.inject.Inject; +import io.swagger.v3.oas.annotations.tags.Tag; +import uk.ac.cam.cl.dtg.isaac.dto.SegueErrorResponse; +import uk.ac.cam.cl.dtg.segue.api.managers.UserAccountManager; +import uk.ac.cam.cl.dtg.segue.auth.exceptions.NoUserLoggedInException; +import uk.ac.cam.cl.dtg.segue.dao.ILogManager; +import uk.ac.cam.cl.dtg.util.AbstractConfigLoader; + +import jakarta.servlet.http.HttpServletRequest; +import jakarta.ws.rs.Consumes; +import jakarta.ws.rs.POST; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import jakarta.ws.rs.core.Context; +import jakarta.ws.rs.core.MediaType; +import jakarta.ws.rs.core.Response; + +@Path("/skills-questions") +@Tag(name = "SkillsQuestionFacade", description = "/skills-questions") +public class SkillsQuestionFacade extends AbstractIsaacFacade { + + private final UserAccountManager userManager; + + @Inject + public SkillsQuestionFacade(final AbstractConfigLoader properties, final UserAccountManager userManager, + final ILogManager logManager) { + super(properties, logManager); + this.userManager = userManager; + } + + @POST + @Path("/{appId}/answer") + @Consumes(MediaType.APPLICATION_JSON) + public Response answerQuestion(@Context final HttpServletRequest request, + @PathParam("appId") final String appId, + final String body) { + try { + userManager.getCurrentRegisteredUser(request); + return Response.ok().build(); + } catch (NoUserLoggedInException e) { + return SegueErrorResponse.getNotLoggedInResponse(); + } + } +} diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsQuestionFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsQuestionFacadeIT.java new file mode 100644 index 0000000000..894a1ae8c4 --- /dev/null +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsQuestionFacadeIT.java @@ -0,0 +1,30 @@ +package uk.ac.cam.cl.dtg.isaac.api; + +import org.junit.jupiter.api.Test; +import uk.ac.cam.cl.dtg.segue.api.AuthenticationFacade; + +import jakarta.ws.rs.core.Response; + +@SuppressWarnings("checkstyle:MissingJavadocType") +public class SkillsQuestionFacadeIT extends IsaacIntegrationTestWithREST { + @Test + public void notLoggedIn_Returns401() throws Exception { + var response = testServer().client().post("/skills-questions/some_app/answer", "{}"); + response.assertError("You must be logged in to access this resource.", Response.Status.UNAUTHORIZED); + } + +// @Test +// public void loggedIn_unknownApp_Returns404() throws Exception { +// var client = testServer().client(); +// client.loginAs(integrationTestUsers.TEST_STUDENT); +// var response = client.post("/skills-questions/unknown_app/answer", "{}"); +// response.assertError("No app found for app ID: unknown_app", Response.Status.NOT_FOUND); +// } + + private TestServer testServer() throws Exception { + return startServer( + new AuthenticationFacade(properties, userAccountManager, logManager, misuseMonitor), + new SkillsQuestionFacade(properties, userAccountManager, logManager) + ); + } +} From 627877459ebf27ec02466c0f04d36021a353ba0b Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Wed, 10 Jun 2026 13:42:24 +0100 Subject: [PATCH 02/31] check that app exists --- .../ac/cam/cl/dtg/isaac/api/SkillsFacade.java | 78 +++++++++++++++++++ .../dtg/isaac/api/SkillsQuestionFacade.java | 46 ----------- .../cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 51 ++++++++++++ .../dtg/isaac/api/SkillsQuestionFacadeIT.java | 30 ------- 4 files changed, 129 insertions(+), 76 deletions(-) create mode 100644 src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java delete mode 100644 src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsQuestionFacade.java create mode 100644 src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java delete mode 100644 src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsQuestionFacadeIT.java diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java new file mode 100644 index 0000000000..f3345a107d --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java @@ -0,0 +1,78 @@ +package uk.ac.cam.cl.dtg.isaac.api; + +import com.google.inject.Inject; +import io.swagger.v3.oas.annotations.tags.Tag; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import uk.ac.cam.cl.dtg.isaac.dto.SegueErrorResponse; +import uk.ac.cam.cl.dtg.segue.api.managers.UserAccountManager; +import uk.ac.cam.cl.dtg.segue.auth.exceptions.NoUserLoggedInException; +import uk.ac.cam.cl.dtg.segue.dao.ILogManager; +import uk.ac.cam.cl.dtg.segue.dao.content.ContentManagerException; +import uk.ac.cam.cl.dtg.segue.dao.content.GitContentManager; +import uk.ac.cam.cl.dtg.util.AbstractConfigLoader; + +import jakarta.servlet.http.HttpServletRequest; +import jakarta.ws.rs.Consumes; +import jakarta.ws.rs.POST; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import jakarta.ws.rs.core.Context; +import jakarta.ws.rs.core.MediaType; +import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.core.Response.Status; + +/** + * Skills Facade + * This facade supports interaction related to the Isaac Skill Practice apps. + */ +@Path("/skills") +@Tag(name = "SkillsFacade", description = "/skills") +public class SkillsFacade extends AbstractIsaacFacade { + private static final Logger log = LoggerFactory.getLogger(SkillsFacade.class); + + private final UserAccountManager userManager; + private final GitContentManager contentManager; + + /** + * Constructor. + */ + @Inject + public SkillsFacade(final AbstractConfigLoader properties, final UserAccountManager userManager, + final ILogManager logManager, final GitContentManager contentManager) { + super(properties, logManager); + this.userManager = userManager; + this.contentManager = contentManager; + } + + /** + * Endpoint that records when a user has answered a question. + * + * @param request + * - the servlet request so we can find out if it is a known user. + * @param appId + * - the app that the attempt belongs to + * @return Response containing a QuestionValidationResponse object or containing a SegueErrorResponse . + */ + @POST + @Path("/{appId}/answer") + @Consumes(MediaType.APPLICATION_JSON) + public Response answerQuestion(@Context final HttpServletRequest request, + @PathParam("appId") final String appId) { + try { + userManager.getCurrentRegisteredUser(request); + if (null == this.contentManager.getContentDOById(appId)) { + var error = new SegueErrorResponse(Status.NOT_FOUND, "No app found for given id: " + appId); + log.warn(error.getErrorMessage()); + return error.toResponse(); + } + return Response.ok().build(); + } catch (final NoUserLoggedInException e) { + return SegueErrorResponse.getNotLoggedInResponse(); + } catch (final ContentManagerException e) { + var error = new SegueErrorResponse(Status.NOT_FOUND, "Error locating the version requested", e); + log.error(error.getErrorMessage(), e); + return error.toResponse(); + } + } +} diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsQuestionFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsQuestionFacade.java deleted file mode 100644 index cd447bd4bf..0000000000 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsQuestionFacade.java +++ /dev/null @@ -1,46 +0,0 @@ -package uk.ac.cam.cl.dtg.isaac.api; - -import com.google.inject.Inject; -import io.swagger.v3.oas.annotations.tags.Tag; -import uk.ac.cam.cl.dtg.isaac.dto.SegueErrorResponse; -import uk.ac.cam.cl.dtg.segue.api.managers.UserAccountManager; -import uk.ac.cam.cl.dtg.segue.auth.exceptions.NoUserLoggedInException; -import uk.ac.cam.cl.dtg.segue.dao.ILogManager; -import uk.ac.cam.cl.dtg.util.AbstractConfigLoader; - -import jakarta.servlet.http.HttpServletRequest; -import jakarta.ws.rs.Consumes; -import jakarta.ws.rs.POST; -import jakarta.ws.rs.Path; -import jakarta.ws.rs.PathParam; -import jakarta.ws.rs.core.Context; -import jakarta.ws.rs.core.MediaType; -import jakarta.ws.rs.core.Response; - -@Path("/skills-questions") -@Tag(name = "SkillsQuestionFacade", description = "/skills-questions") -public class SkillsQuestionFacade extends AbstractIsaacFacade { - - private final UserAccountManager userManager; - - @Inject - public SkillsQuestionFacade(final AbstractConfigLoader properties, final UserAccountManager userManager, - final ILogManager logManager) { - super(properties, logManager); - this.userManager = userManager; - } - - @POST - @Path("/{appId}/answer") - @Consumes(MediaType.APPLICATION_JSON) - public Response answerQuestion(@Context final HttpServletRequest request, - @PathParam("appId") final String appId, - final String body) { - try { - userManager.getCurrentRegisteredUser(request); - return Response.ok().build(); - } catch (NoUserLoggedInException e) { - return SegueErrorResponse.getNotLoggedInResponse(); - } - } -} diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java new file mode 100644 index 0000000000..428a475f2e --- /dev/null +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -0,0 +1,51 @@ +package uk.ac.cam.cl.dtg.isaac.api; + +import org.junit.jupiter.api.Test; +import uk.ac.cam.cl.dtg.segue.api.AuthenticationFacade; +import uk.ac.cam.cl.dtg.segue.dao.content.GitContentManager; +import uk.ac.cam.cl.dtg.segue.search.ElasticSearchProvider; + +import jakarta.ws.rs.core.Response; +import java.net.UnknownHostException; + +@SuppressWarnings("checkstyle:MissingJavadocType") +public class SkillsFacadeIT extends IsaacIntegrationTestWithREST { + @Test + public void notLoggedIn_Returns401() throws Exception { + var response = testServer().client().post("/skills/unknown_app/answer", "{}"); + response.assertError("You must be logged in to access this resource.", Response.Status.UNAUTHORIZED); + } + + @Test + public void loggedIn_elasticsearchUnavailable_Returns503() throws Exception { + var client = testServer(brokenContentManager()).client(); + client.loginAs(integrationTestUsers.TEST_STUDENT); + var response = client.post("/skills/unknown_app/answer", "{}"); + response.assertError("Error locating the version requested", Response.Status.NOT_FOUND); + } + + @Test + public void loggedIn_unknownApp_Returns404() throws Exception { + var client = testServer().client(); + client.loginAs(integrationTestUsers.TEST_STUDENT); + var response = client.post("/skills/unknown_app/answer", "{}"); + response.assertError("No app found for given id: unknown_app", Response.Status.NOT_FOUND); + } + + private GitContentManager brokenContentManager() throws UnknownHostException { + var brokenClient = ElasticSearchProvider.getClient("localhost", 1, "elastic", "elastic"); + var brokenProvider = new ElasticSearchProvider(brokenClient); + return new GitContentManager(null, brokenProvider, mainMapper, contentMapper, properties); + } + + private TestServer testServer() throws Exception { + return testServer(contentManager); + } + + private TestServer testServer(final GitContentManager cm) throws Exception { + return startServer( + new AuthenticationFacade(properties, userAccountManager, logManager, misuseMonitor), + new SkillsFacade(properties, userAccountManager, logManager, cm) + ); + } +} diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsQuestionFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsQuestionFacadeIT.java deleted file mode 100644 index 894a1ae8c4..0000000000 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsQuestionFacadeIT.java +++ /dev/null @@ -1,30 +0,0 @@ -package uk.ac.cam.cl.dtg.isaac.api; - -import org.junit.jupiter.api.Test; -import uk.ac.cam.cl.dtg.segue.api.AuthenticationFacade; - -import jakarta.ws.rs.core.Response; - -@SuppressWarnings("checkstyle:MissingJavadocType") -public class SkillsQuestionFacadeIT extends IsaacIntegrationTestWithREST { - @Test - public void notLoggedIn_Returns401() throws Exception { - var response = testServer().client().post("/skills-questions/some_app/answer", "{}"); - response.assertError("You must be logged in to access this resource.", Response.Status.UNAUTHORIZED); - } - -// @Test -// public void loggedIn_unknownApp_Returns404() throws Exception { -// var client = testServer().client(); -// client.loginAs(integrationTestUsers.TEST_STUDENT); -// var response = client.post("/skills-questions/unknown_app/answer", "{}"); -// response.assertError("No app found for app ID: unknown_app", Response.Status.NOT_FOUND); -// } - - private TestServer testServer() throws Exception { - return startServer( - new AuthenticationFacade(properties, userAccountManager, logManager, misuseMonitor), - new SkillsQuestionFacade(properties, userAccountManager, logManager) - ); - } -} From 7a434ed8ed158183991a2eb51d45cb8bb21d6e91 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Wed, 10 Jun 2026 13:54:29 +0100 Subject: [PATCH 03/31] clean up comments --- src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java index f3345a107d..69fe746093 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java @@ -23,8 +23,7 @@ import jakarta.ws.rs.core.Response.Status; /** - * Skills Facade - * This facade supports interaction related to the Isaac Skill Practice apps. + * Skills Facade, supports interaction related to the Isaac Skill Practice apps. */ @Path("/skills") @Tag(name = "SkillsFacade", description = "/skills") @@ -52,7 +51,6 @@ public SkillsFacade(final AbstractConfigLoader properties, final UserAccountMana * - the servlet request so we can find out if it is a known user. * @param appId * - the app that the attempt belongs to - * @return Response containing a QuestionValidationResponse object or containing a SegueErrorResponse . */ @POST @Path("/{appId}/answer") From 45018e8b01300272f323b55aeccbaf1c2035413f Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Wed, 10 Jun 2026 14:15:55 +0100 Subject: [PATCH 04/31] check that app_id is a valid AnvilApp --- .../java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java | 3 ++- .../uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 10 ++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java index 69fe746093..13647d7b88 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java @@ -4,6 +4,7 @@ import io.swagger.v3.oas.annotations.tags.Tag; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import uk.ac.cam.cl.dtg.isaac.dos.content.AnvilApp; import uk.ac.cam.cl.dtg.isaac.dto.SegueErrorResponse; import uk.ac.cam.cl.dtg.segue.api.managers.UserAccountManager; import uk.ac.cam.cl.dtg.segue.auth.exceptions.NoUserLoggedInException; @@ -59,7 +60,7 @@ public Response answerQuestion(@Context final HttpServletRequest request, @PathParam("appId") final String appId) { try { userManager.getCurrentRegisteredUser(request); - if (null == this.contentManager.getContentDOById(appId)) { + if (!(this.contentManager.getContentDOById(appId) instanceof AnvilApp)) { var error = new SegueErrorResponse(Status.NOT_FOUND, "No app found for given id: " + appId); log.warn(error.getErrorMessage()); return error.toResponse(); diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index 428a475f2e..41c07ff6be 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -2,6 +2,8 @@ import org.junit.jupiter.api.Test; import uk.ac.cam.cl.dtg.segue.api.AuthenticationFacade; + +import static uk.ac.cam.cl.dtg.isaac.api.ITConstants.REGRESSION_TEST_PAGE_ID; import uk.ac.cam.cl.dtg.segue.dao.content.GitContentManager; import uk.ac.cam.cl.dtg.segue.search.ElasticSearchProvider; @@ -32,6 +34,14 @@ public void loggedIn_unknownApp_Returns404() throws Exception { response.assertError("No app found for given id: unknown_app", Response.Status.NOT_FOUND); } + @Test + public void loggedIn_idMatchesNonApp_Returns404() throws Exception { + var client = testServer().client(); + client.loginAs(integrationTestUsers.TEST_STUDENT); + var response = client.post("/skills/" + REGRESSION_TEST_PAGE_ID + "/answer", "{}"); + response.assertError("No app found for given id: " + REGRESSION_TEST_PAGE_ID, Response.Status.NOT_FOUND); + } + private GitContentManager brokenContentManager() throws UnknownHostException { var brokenClient = ElasticSearchProvider.getClient("localhost", 1, "elastic", "elastic"); var brokenProvider = new ElasticSearchProvider(brokenClient); From 767fd0ae0b1a18db0437afe1428c642a262b8131 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Wed, 10 Jun 2026 15:14:49 +0100 Subject: [PATCH 05/31] extract ElasticSearchHelper --- .../isaac/api/ElasticSearchTestHelper.java | 47 ++++++++++++++++++ .../api/IsaacIntegrationTestWithREST.java | 3 ++ .../cl/dtg/isaac/api/QuestionFacadeIT.java | 48 ++++--------------- 3 files changed, 58 insertions(+), 40 deletions(-) create mode 100644 src/test/java/uk/ac/cam/cl/dtg/isaac/api/ElasticSearchTestHelper.java diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/ElasticSearchTestHelper.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/ElasticSearchTestHelper.java new file mode 100644 index 0000000000..66d8998a23 --- /dev/null +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/ElasticSearchTestHelper.java @@ -0,0 +1,47 @@ +package uk.ac.cam.cl.dtg.isaac.api; + +import org.json.JSONObject; +import uk.ac.cam.cl.dtg.isaac.dos.content.Content; +import uk.ac.cam.cl.dtg.segue.dao.content.ContentSubclassMapper; +import uk.ac.cam.cl.dtg.segue.dao.content.GitContentManager; +import uk.ac.cam.cl.dtg.segue.etl.ElasticSearchIndexer; + +import java.util.List; + +import static org.testcontainers.shaded.com.google.common.collect.Maps.immutableEntry; + +@SuppressWarnings("checkstyle:MissingJavadocType") +public class ElasticSearchTestHelper { + private final ElasticSearchIndexer elasticSearchProvider; + private final GitContentManager contentManager; + private final ContentSubclassMapper contentMapper; + + public ElasticSearchTestHelper(final ElasticSearchIndexer elasticSearchProvider, + final GitContentManager contentManager, + final ContentSubclassMapper contentMapper) { + this.elasticSearchProvider = elasticSearchProvider; + this.contentManager = contentManager; + this.contentMapper = contentMapper; + } + + public T persist(final T content) throws Exception { + elasticSearchProvider.bulkIndexWithIDs( + contentManager.getCurrentContentSHA(), + "content", + List.of(immutableEntry( + content.getId(), contentMapper.getSharedContentObjectMapper().writeValueAsString(content)) + ) + ); + return content; + } + + public JSONObject persistJSON(final JSONObject contentJSON) throws Exception { + contentJSON.put("id", "i1"); + elasticSearchProvider.bulkIndexWithIDs( + contentManager.getCurrentContentSHA(), + "content", + List.of(immutableEntry("i1", contentJSON.toString())) + ); + return contentJSON; + } +} diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java index f767c08fa6..a56d88299e 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java @@ -40,6 +40,9 @@ * interacting with the server. As the server runs in-process, mocking and debugging still work. */ public class IsaacIntegrationTestWithREST extends AbstractIsaacIntegrationTest { + static final ElasticSearchTestHelper elasticHelper = + new ElasticSearchTestHelper(elasticSearchProvider, contentManager, contentMapper); + private final Set cleanups = new HashSet<>(); @SuppressWarnings({"checkstyle:EmptyCatchBlock", "checkstyle:MissingJavadocMethod"}) diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/QuestionFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/QuestionFacadeIT.java index f1eaed5b9b..ceb070634c 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/QuestionFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/QuestionFacadeIT.java @@ -1,31 +1,21 @@ package uk.ac.cam.cl.dtg.isaac.api; -import com.google.inject.Injector; import org.json.JSONArray; import org.json.JSONObject; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; -import uk.ac.cam.cl.dtg.isaac.dos.IsaacDndQuestion; import uk.ac.cam.cl.dtg.isaac.dos.content.Content; import uk.ac.cam.cl.dtg.isaac.dos.content.DndChoice; -import uk.ac.cam.cl.dtg.isaac.quiz.IsaacDndValidator; -import uk.ac.cam.cl.dtg.isaac.quiz.IsaacDndValidatorTest; -import uk.ac.cam.cl.dtg.isaac.quiz.IsaacStringMatchValidator; import uk.ac.cam.cl.dtg.segue.api.QuestionFacade; -import uk.ac.cam.cl.dtg.segue.configuration.SegueGuiceConfigurationModule; import jakarta.ws.rs.core.Response; import java.util.List; import java.util.Map; -import static org.easymock.EasyMock.createNiceMock; -import static org.easymock.EasyMock.expect; -import static org.easymock.EasyMock.replay; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.testcontainers.shaded.com.google.common.collect.Maps.immutableEntry; import static uk.ac.cam.cl.dtg.isaac.quiz.IsaacDndValidatorTest.*; @SuppressWarnings("checkstyle:MissingJavadocType") @@ -71,7 +61,7 @@ public void rightAnswer() throws Exception { class DndQuestion { @Test public void invalidQuestion() throws Exception { - var question = persistJSON(new JSONObject() + var question = elasticHelper.persistJSON(new JSONObject() .put("type", "isaacDndQuestion") .put("items", new JSONArray().put(new JSONObject().put("id", "item_id").put("type", "item"))) .put("children", new JSONArray().put( @@ -109,7 +99,7 @@ public void invalidQuestion() throws Exception { public void badRequest_ErrorReturned( final String answerStr, final String emsg, final String estate ) throws Exception { - var dndQuestion = persist(question(correctChoice(item(item_3cm, "leg_1")))); + var dndQuestion = elasticHelper.persist(question(correctChoice(item(item_3cm, "leg_1")))); var response = testServer().client().post(url(dndQuestion.getId()), answerStr); response.assertError(emsg, estate); } @@ -124,7 +114,7 @@ public void badRequest_ErrorReturned( public void badRequest_IncorrectReturnedWithExplanation( final String answerStr, final String emsg ) throws Exception { - var dndQuestion = persist(question( + var dndQuestion = elasticHelper.persist(question( correctChoice(item(item_3cm, "leg_1"), item(item_4cm, "leg_2"), item(item_5cm, "hypothenuse")) )); var response = testServer().client().post(url(dndQuestion.getId()), answerStr).readEntityAsJson(); @@ -138,7 +128,7 @@ public void badRequest_IncorrectReturnedWithExplanation( "{\"type\": \"dndChoice\", \"items\": []}" }, delimiter = '|') public void emptyAnswer_IncorrectReturned(final String answerStr) throws Exception { - var dndQuestion = persist(question(correctChoice(item(item_3cm, "leg_1")))); + var dndQuestion = elasticHelper.persist(question(correctChoice(item(item_3cm, "leg_1")))); var response = testServer().client().post(url(dndQuestion.getId()), answerStr).readEntityAsJson(); assertFalse(response.getBoolean("correct")); assertEquals( @@ -158,7 +148,7 @@ public void emptyAnswer_IncorrectReturned(final String answerStr) throws Excepti public void correctAnswer_CorrectReturned(final String answerStr) throws Exception { var dndQuestion = question(correctChoice(item(item_3cm, "leg_1"))); dndQuestion.setChildren(List.of(new Content("[drop-zone:leg_1]"))); - persist(dndQuestion); + elasticHelper.persist(dndQuestion); var response = testServer().client().post(url(dndQuestion.getId()), answerStr).readEntityAsJson(); assertTrue(response.getBoolean("correct")); @@ -168,7 +158,7 @@ public void correctAnswer_CorrectReturned(final String answerStr) throws Excepti @Test public void wrongAnswer_IncorrectReturned() throws Exception { - var dndQuestion = persist(question( + var dndQuestion = elasticHelper.persist(question( correctChoice(item(item_3cm, "leg_1"), item(item_4cm, "leg_2"), item(item_5cm, "hypothenuse")) )); var answer = choice(item(item_3cm, "leg_2"), item(item_4cm, "hypothenuse"), item(item_5cm, "leg_1")); @@ -182,7 +172,7 @@ public void wrongAnswer_IncorrectReturned() throws Exception { @Test public void answerWithMatchingExplanation_ExplanationReturned() throws Exception { var explanation = new Content("That's right!"); - var dndQuestion = persist(question(correctChoice( + var dndQuestion = elasticHelper.persist(question(correctChoice( explanation, item(item_3cm, "leg_1"), item(item_4cm, "leg_2"), item(item_5cm, "hypothenuse") ))); @@ -200,7 +190,7 @@ public void detailedItemFeedbackRequested_DropZonesCorrectReturned() throws Exce correctChoice(item(item_3cm, "leg_1"), item(item_4cm, "leg_2"), item(item_5cm, "hypothenuse")) ); dndQuestion.setDetailedItemFeedback(true); - persist(dndQuestion); + elasticHelper.persist(dndQuestion); var answer = choice(item(item_3cm, "leg_1"), item(item_4cm, "leg_2"), item(item_5cm, "hypothenuse")); var response = testServer().client().post(url(dndQuestion.getId()), answer).readEntityAsJson(); @@ -213,28 +203,6 @@ public void detailedItemFeedbackRequested_DropZonesCorrectReturned() throws Exce } } - private IsaacDndQuestion persist(final IsaacDndQuestion question) throws Exception { - elasticSearchProvider.bulkIndexWithIDs( - contentManager.getCurrentContentSHA(), - "content", - List.of(immutableEntry( - question.getId(), contentMapper.getSharedContentObjectMapper().writeValueAsString(question)) - ) - ); - return question; - } - - private JSONObject persistJSON(final JSONObject questionJSON) throws Exception { - questionJSON.put("id", "i1"); - elasticSearchProvider.bulkIndexWithIDs( - contentManager.getCurrentContentSHA(), - "content", - List.of(immutableEntry("i1", questionJSON.toString())) - ); - return questionJSON; - } - - private TestServer testServer() throws Exception { return startServer( new QuestionFacade(properties, contentMapper, contentManager, userAccountManager, From db449c7436be8b22635e3f21d3b405b0bf3bd472 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Wed, 10 Jun 2026 16:12:21 +0100 Subject: [PATCH 06/31] add test stub for happy path --- .../java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index 41c07ff6be..bf0afcb301 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -1,5 +1,6 @@ package uk.ac.cam.cl.dtg.isaac.api; +import org.json.JSONObject; import org.junit.jupiter.api.Test; import uk.ac.cam.cl.dtg.segue.api.AuthenticationFacade; @@ -42,6 +43,14 @@ public void loggedIn_idMatchesNonApp_Returns404() throws Exception { response.assertError("No app found for given id: " + REGRESSION_TEST_PAGE_ID, Response.Status.NOT_FOUND); } + @Test + public void happy_happy() throws Exception { + var app = elasticHelper.persistJSON(new JSONObject().put("type", "anvilApp")); + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); + var response = client.post("/skills/" + app.getString("id") + "/answer", "{}"); + response.readEntity(String.class); + } + private GitContentManager brokenContentManager() throws UnknownHostException { var brokenClient = ElasticSearchProvider.getClient("localhost", 1, "elastic", "elastic"); var brokenProvider = new ElasticSearchProvider(brokenClient); From 220cf77a886068b81d063322db2778353f9c5eb1 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Wed, 10 Jun 2026 17:23:30 +0100 Subject: [PATCH 07/31] introduce validation for payload --- .../ac/cam/cl/dtg/isaac/api/SkillsFacade.java | 13 +++++-- .../InvalidMarkingResponseException.java | 13 +++++++ .../dtg/isaac/api/managers/SkillsManager.java | 34 +++++++++++++++++++ .../isaac/dto/AnvilMarkingResponseDTO.java | 30 ++++++++++++++++ .../cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 25 ++++++++++---- 5 files changed, 106 insertions(+), 9 deletions(-) create mode 100644 src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/InvalidMarkingResponseException.java create mode 100644 src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java create mode 100644 src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilMarkingResponseDTO.java diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java index 13647d7b88..d3c4c6d057 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java @@ -4,6 +4,8 @@ import io.swagger.v3.oas.annotations.tags.Tag; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import uk.ac.cam.cl.dtg.isaac.api.managers.InvalidMarkingResponseException; +import uk.ac.cam.cl.dtg.isaac.api.managers.SkillsManager; import uk.ac.cam.cl.dtg.isaac.dos.content.AnvilApp; import uk.ac.cam.cl.dtg.isaac.dto.SegueErrorResponse; import uk.ac.cam.cl.dtg.segue.api.managers.UserAccountManager; @@ -33,16 +35,19 @@ public class SkillsFacade extends AbstractIsaacFacade { private final UserAccountManager userManager; private final GitContentManager contentManager; + private final SkillsManager skillsManager; /** * Constructor. */ @Inject public SkillsFacade(final AbstractConfigLoader properties, final UserAccountManager userManager, - final ILogManager logManager, final GitContentManager contentManager) { + final ILogManager logManager, final GitContentManager contentManager, + final SkillsManager skillsManager) { super(properties, logManager); this.userManager = userManager; this.contentManager = contentManager; + this.skillsManager = skillsManager; } /** @@ -57,9 +62,11 @@ public SkillsFacade(final AbstractConfigLoader properties, final UserAccountMana @Path("/{appId}/answer") @Consumes(MediaType.APPLICATION_JSON) public Response answerQuestion(@Context final HttpServletRequest request, - @PathParam("appId") final String appId) { + @PathParam("appId") final String appId, + final String body) { try { userManager.getCurrentRegisteredUser(request); + skillsManager.parseResponse(body); if (!(this.contentManager.getContentDOById(appId) instanceof AnvilApp)) { var error = new SegueErrorResponse(Status.NOT_FOUND, "No app found for given id: " + appId); log.warn(error.getErrorMessage()); @@ -68,6 +75,8 @@ public Response answerQuestion(@Context final HttpServletRequest request, return Response.ok().build(); } catch (final NoUserLoggedInException e) { return SegueErrorResponse.getNotLoggedInResponse(); + } catch (final InvalidMarkingResponseException e) { + return new SegueErrorResponse(Status.BAD_REQUEST, e.getMessage()).toResponse(); } catch (final ContentManagerException e) { var error = new SegueErrorResponse(Status.NOT_FOUND, "Error locating the version requested", e); log.error(error.getErrorMessage(), e); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/InvalidMarkingResponseException.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/InvalidMarkingResponseException.java new file mode 100644 index 0000000000..e234c462c3 --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/InvalidMarkingResponseException.java @@ -0,0 +1,13 @@ +package uk.ac.cam.cl.dtg.isaac.api.managers; + +/** + * An exception that indicates an invalid marking response was received from the external marking server. + */ +public class InvalidMarkingResponseException extends Exception { + /** + * Constructor with message. + */ + public InvalidMarkingResponseException(final String message) { + super(message); + } +} diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java new file mode 100644 index 0000000000..882a569365 --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java @@ -0,0 +1,34 @@ +package uk.ac.cam.cl.dtg.isaac.api.managers; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.inject.Inject; +import uk.ac.cam.cl.dtg.isaac.dto.AnvilMarkingResponseDTO; + +/** + * Manager for Isaac Skills Practice app interactions. + */ +public class SkillsManager { + private static final ObjectMapper objectMapper = new ObjectMapper(); + + /** + * Constructor. + */ + @Inject + public SkillsManager() { } + + /** + * Parses and validates the raw JSON body from the external marking server. + * + * @param body - the raw JSON request body + * @return the deserialised marking response + * @throws InvalidMarkingResponseException if the body is missing or malformed + */ + public AnvilMarkingResponseDTO parseResponse(final String body) throws InvalidMarkingResponseException { + try { + return objectMapper.readValue(body, AnvilMarkingResponseDTO.class); + } catch (final JsonProcessingException e) { + throw new InvalidMarkingResponseException("Invalid JSON object submitted"); + } + } +} diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilMarkingResponseDTO.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilMarkingResponseDTO.java new file mode 100644 index 0000000000..bf3176aaed --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilMarkingResponseDTO.java @@ -0,0 +1,30 @@ +package uk.ac.cam.cl.dtg.isaac.dto; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; + +/** + * DTO representing the response received from the external Anvil marking server. + */ +public class AnvilMarkingResponseDTO { + private final String payload; + + /** + * Constructor. + * + * @param payload - the signed payload from the marking server + */ + @JsonCreator + public AnvilMarkingResponseDTO(@JsonProperty(value = "payload", required = true) final String payload) { + this.payload = payload; + } + + /** + * Gets the payload. + * + * @return the signed payload string + */ + public String getPayload() { + return payload; + } +} diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index bf0afcb301..e2929b0baf 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -2,17 +2,20 @@ import org.json.JSONObject; import org.junit.jupiter.api.Test; +import uk.ac.cam.cl.dtg.isaac.api.managers.SkillsManager; import uk.ac.cam.cl.dtg.segue.api.AuthenticationFacade; - -import static uk.ac.cam.cl.dtg.isaac.api.ITConstants.REGRESSION_TEST_PAGE_ID; import uk.ac.cam.cl.dtg.segue.dao.content.GitContentManager; import uk.ac.cam.cl.dtg.segue.search.ElasticSearchProvider; import jakarta.ws.rs.core.Response; import java.net.UnknownHostException; +import static uk.ac.cam.cl.dtg.isaac.api.ITConstants.REGRESSION_TEST_PAGE_ID; + @SuppressWarnings("checkstyle:MissingJavadocType") public class SkillsFacadeIT extends IsaacIntegrationTestWithREST { + private static final String VALID_BODY = new JSONObject().put("payload", "some_signed_payload").toString(); + @Test public void notLoggedIn_Returns401() throws Exception { var response = testServer().client().post("/skills/unknown_app/answer", "{}"); @@ -23,7 +26,7 @@ public void notLoggedIn_Returns401() throws Exception { public void loggedIn_elasticsearchUnavailable_Returns503() throws Exception { var client = testServer(brokenContentManager()).client(); client.loginAs(integrationTestUsers.TEST_STUDENT); - var response = client.post("/skills/unknown_app/answer", "{}"); + var response = client.post("/skills/unknown_app/answer", VALID_BODY); response.assertError("Error locating the version requested", Response.Status.NOT_FOUND); } @@ -31,7 +34,7 @@ public void loggedIn_elasticsearchUnavailable_Returns503() throws Exception { public void loggedIn_unknownApp_Returns404() throws Exception { var client = testServer().client(); client.loginAs(integrationTestUsers.TEST_STUDENT); - var response = client.post("/skills/unknown_app/answer", "{}"); + var response = client.post("/skills/unknown_app/answer", VALID_BODY); response.assertError("No app found for given id: unknown_app", Response.Status.NOT_FOUND); } @@ -39,15 +42,23 @@ public void loggedIn_unknownApp_Returns404() throws Exception { public void loggedIn_idMatchesNonApp_Returns404() throws Exception { var client = testServer().client(); client.loginAs(integrationTestUsers.TEST_STUDENT); - var response = client.post("/skills/" + REGRESSION_TEST_PAGE_ID + "/answer", "{}"); + var response = client.post("/skills/" + REGRESSION_TEST_PAGE_ID + "/answer", VALID_BODY); response.assertError("No app found for given id: " + REGRESSION_TEST_PAGE_ID, Response.Status.NOT_FOUND); } @Test - public void happy_happy() throws Exception { + public void loggedIn_validApp_missingPayload_Returns400() throws Exception { var app = elasticHelper.persistJSON(new JSONObject().put("type", "anvilApp")); var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); var response = client.post("/skills/" + app.getString("id") + "/answer", "{}"); + response.assertError("Invalid JSON object submitted", Response.Status.BAD_REQUEST); + } + + @Test + public void happy_happy() throws Exception { + var app = elasticHelper.persistJSON(new JSONObject().put("type", "anvilApp")); + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); + var response = client.post("/skills/" + app.getString("id") + "/answer", VALID_BODY); response.readEntity(String.class); } @@ -64,7 +75,7 @@ private TestServer testServer() throws Exception { private TestServer testServer(final GitContentManager cm) throws Exception { return startServer( new AuthenticationFacade(properties, userAccountManager, logManager, misuseMonitor), - new SkillsFacade(properties, userAccountManager, logManager, cm) + new SkillsFacade(properties, userAccountManager, logManager, cm, new SkillsManager()) ); } } From b0b2d76dec567e178a0837084e1e5e86c6a08e90 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Wed, 10 Jun 2026 17:50:38 +0100 Subject: [PATCH 08/31] add two test cases for request payload validation --- .../cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 80 ++++++++++++------- 1 file changed, 52 insertions(+), 28 deletions(-) diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index e2929b0baf..bb104226dd 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -1,6 +1,7 @@ package uk.ac.cam.cl.dtg.isaac.api; import org.json.JSONObject; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import uk.ac.cam.cl.dtg.isaac.api.managers.SkillsManager; import uk.ac.cam.cl.dtg.segue.api.AuthenticationFacade; @@ -22,43 +23,61 @@ public void notLoggedIn_Returns401() throws Exception { response.assertError("You must be logged in to access this resource.", Response.Status.UNAUTHORIZED); } - @Test - public void loggedIn_elasticsearchUnavailable_Returns503() throws Exception { - var client = testServer(brokenContentManager()).client(); - client.loginAs(integrationTestUsers.TEST_STUDENT); - var response = client.post("/skills/unknown_app/answer", VALID_BODY); - response.assertError("Error locating the version requested", Response.Status.NOT_FOUND); - } + @Nested + class AppIdCheck { + @Test + public void elasticsearchUnavailable_Returns404() throws Exception { + var client = testServer(brokenContentManager()).client(); + client.loginAs(integrationTestUsers.TEST_STUDENT); + var response = client.post("/skills/unknown_app/answer", VALID_BODY); + response.assertError("Error locating the version requested", Response.Status.NOT_FOUND); + } - @Test - public void loggedIn_unknownApp_Returns404() throws Exception { - var client = testServer().client(); - client.loginAs(integrationTestUsers.TEST_STUDENT); - var response = client.post("/skills/unknown_app/answer", VALID_BODY); - response.assertError("No app found for given id: unknown_app", Response.Status.NOT_FOUND); - } + @Test + public void unknownApp_Returns404() throws Exception { + var client = testServer().client(); + client.loginAs(integrationTestUsers.TEST_STUDENT); + var response = client.post("/skills/unknown_app/answer", VALID_BODY); + response.assertError("No app found for given id: unknown_app", Response.Status.NOT_FOUND); + } - @Test - public void loggedIn_idMatchesNonApp_Returns404() throws Exception { - var client = testServer().client(); - client.loginAs(integrationTestUsers.TEST_STUDENT); - var response = client.post("/skills/" + REGRESSION_TEST_PAGE_ID + "/answer", VALID_BODY); - response.assertError("No app found for given id: " + REGRESSION_TEST_PAGE_ID, Response.Status.NOT_FOUND); + @Test + public void idMatchesNonApp_Returns404() throws Exception { + var client = testServer().client(); + client.loginAs(integrationTestUsers.TEST_STUDENT); + var response = client.post("/skills/" + REGRESSION_TEST_PAGE_ID + "/answer", VALID_BODY); + response.assertError("No app found for given id: " + REGRESSION_TEST_PAGE_ID, Response.Status.NOT_FOUND); + } } - @Test - public void loggedIn_validApp_missingPayload_Returns400() throws Exception { - var app = elasticHelper.persistJSON(new JSONObject().put("type", "anvilApp")); - var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var response = client.post("/skills/" + app.getString("id") + "/answer", "{}"); - response.assertError("Invalid JSON object submitted", Response.Status.BAD_REQUEST); + @Nested + class RequestPayloadCheck { + @Test + public void missingPayload_Returns400() throws Exception { + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); + var response = client.post(validUrl(), "{}"); + response.assertError("Invalid JSON object submitted", Response.Status.BAD_REQUEST); + } + + @Test + public void numericPayload_Returns200() throws Exception { + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); + var response = client.post(validUrl(), new JSONObject().put("payload", 123).toString()); + response.readEntity(String.class); + } + + @Test + public void extraAttribute_Returns400() throws Exception { + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); + var response = client.post(validUrl(), new JSONObject().put("payload", "some_signed_payload").put("extra", "value").toString()); + response.assertError("Invalid JSON object submitted", Response.Status.BAD_REQUEST); + } } @Test public void happy_happy() throws Exception { - var app = elasticHelper.persistJSON(new JSONObject().put("type", "anvilApp")); var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var response = client.post("/skills/" + app.getString("id") + "/answer", VALID_BODY); + var response = client.post(validUrl(), VALID_BODY); response.readEntity(String.class); } @@ -68,6 +87,11 @@ private GitContentManager brokenContentManager() throws UnknownHostException { return new GitContentManager(null, brokenProvider, mainMapper, contentMapper, properties); } + private String validUrl() throws Exception { + var app = elasticHelper.persistJSON(new JSONObject().put("type", "anvilApp")); + return "/skills/" + app.getString("id") + "/answer"; + } + private TestServer testServer() throws Exception { return testServer(contentManager); } From e567f8a653fae8f294e11eb449d31871eeac7414 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Mon, 15 Jun 2026 14:10:26 +0100 Subject: [PATCH 09/31] validate HMAC --- .../ac/cam/cl/dtg/isaac/api/SkillsFacade.java | 5 +- .../dtg/isaac/api/managers/SkillsManager.java | 24 +++++- .../isaac/dto/AnvilMarkingResponseDTO.java | 16 +++- .../uk/ac/cam/cl/dtg/segue/api/Constants.java | 1 + .../api/IsaacIntegrationTestWithREST.java | 3 +- .../cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 76 +++++++++++++++++-- .../segue-integration-test-config.yaml | 1 + 7 files changed, 116 insertions(+), 10 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java index d3c4c6d057..e29092a3c0 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java @@ -66,7 +66,10 @@ public Response answerQuestion(@Context final HttpServletRequest request, final String body) { try { userManager.getCurrentRegisteredUser(request); - skillsManager.parseResponse(body); + var markingResponse = skillsManager.parseResponse(body); + if (!skillsManager.isHmacValid(markingResponse)) { + return new SegueErrorResponse(Status.BAD_REQUEST, "Invalid HMAC signature").toResponse(); + } if (!(this.contentManager.getContentDOById(appId) instanceof AnvilApp)) { var error = new SegueErrorResponse(Status.NOT_FOUND, "No app found for given id: " + appId); log.warn(error.getErrorMessage()); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java index 882a569365..8a12fdd9e1 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java @@ -4,6 +4,11 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.inject.Inject; import uk.ac.cam.cl.dtg.isaac.dto.AnvilMarkingResponseDTO; +import uk.ac.cam.cl.dtg.segue.api.managers.UserAuthenticationManager; +import uk.ac.cam.cl.dtg.util.AbstractConfigLoader; + +import static uk.ac.cam.cl.dtg.segue.api.Constants.*; + /** * Manager for Isaac Skills Practice app interactions. @@ -11,11 +16,17 @@ public class SkillsManager { private static final ObjectMapper objectMapper = new ObjectMapper(); + private final String hmacSecret; + /** * Constructor. + * + * @param properties - application configuration */ @Inject - public SkillsManager() { } + public SkillsManager(final AbstractConfigLoader properties) { + this.hmacSecret = properties.getProperty(SKILLS_HMAC_SECRET); + } /** * Parses and validates the raw JSON body from the external marking server. @@ -31,4 +42,15 @@ public AnvilMarkingResponseDTO parseResponse(final String body) throws InvalidMa throw new InvalidMarkingResponseException("Invalid JSON object submitted"); } } + + /** + * Verifies that the HMAC in the DTO matches the expected signature of the payload. + * + * @param dto - the parsed marking response + * @return whether the hmac was valid + */ + public boolean isHmacValid(final AnvilMarkingResponseDTO dto) { + String expected = UserAuthenticationManager.calculateHMAC(hmacSecret, dto.getPayload()); + return expected.equals(dto.getHmac()); + } } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilMarkingResponseDTO.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilMarkingResponseDTO.java index bf3176aaed..a73ba2d8f3 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilMarkingResponseDTO.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilMarkingResponseDTO.java @@ -8,15 +8,20 @@ */ public class AnvilMarkingResponseDTO { private final String payload; + private final String hmac; /** * Constructor. * * @param payload - the signed payload from the marking server + * @param hmac - the HMAC-SHA256 hex digest authenticating the payload */ @JsonCreator - public AnvilMarkingResponseDTO(@JsonProperty(value = "payload", required = true) final String payload) { + public AnvilMarkingResponseDTO( + @JsonProperty(value = "payload", required = true) final String payload, + @JsonProperty(value = "hmac", required = true) final String hmac) { this.payload = payload; + this.hmac = hmac; } /** @@ -27,4 +32,13 @@ public AnvilMarkingResponseDTO(@JsonProperty(value = "payload", required = true) public String getPayload() { return payload; } + + /** + * Gets the HMAC signature. + * + * @return the HMAC-SHA256 hex digest + */ + public String getHmac() { + return hmac; + } } diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/api/Constants.java b/src/main/java/uk/ac/cam/cl/dtg/segue/api/Constants.java index 5a6e27249b..08c28a58a2 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/api/Constants.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/api/Constants.java @@ -192,6 +192,7 @@ public enum AuthenticationCaveat { * Constant representing the key for the HMAC Salt - used in HMAC calculations. */ public static final String HMAC_SALT = "HMAC_SALT"; + public static final String SKILLS_HMAC_SECRET = "SKILLS_HMAC_SECRET"; public static final int TRUNCATED_TOKEN_LENGTH = 8; // Search stuff diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java index a56d88299e..a6650398ce 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java @@ -157,7 +157,8 @@ public TestResponse get(final String url) { public TestResponse post(final String url, final Object body) { Invocation.Builder request = client.target(baseUrl + url).request(MediaType.APPLICATION_JSON); - Response response = builder.apply(request).post(Entity.json(body)); + Object serializable = body instanceof JSONObject j ? j.toString() : body; + Response response = builder.apply(request).post(Entity.json(serializable)); registerCleanup.accept(response::close); return new TestResponse(response); } diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index bb104226dd..01d6868493 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -1,5 +1,6 @@ package uk.ac.cam.cl.dtg.isaac.api; +import org.apache.commons.codec.binary.Base64; import org.json.JSONObject; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -9,13 +10,18 @@ import uk.ac.cam.cl.dtg.segue.search.ElasticSearchProvider; import jakarta.ws.rs.core.Response; -import java.net.UnknownHostException; +import javax.crypto.Mac; +import javax.crypto.spec.SecretKeySpec; import static uk.ac.cam.cl.dtg.isaac.api.ITConstants.REGRESSION_TEST_PAGE_ID; @SuppressWarnings("checkstyle:MissingJavadocType") public class SkillsFacadeIT extends IsaacIntegrationTestWithREST { - private static final String VALID_BODY = new JSONObject().put("payload", "some_signed_payload").toString(); + private static final String HMAC_SECRET = "integration-test-anvil-hmac-secret"; + private static final String VALID_PAYLOAD = "some_signed_payload"; + private static final String HMAC_SHA_256 = "HmacSHA256"; + private static final String VALID_HMAC = sign(HMAC_SECRET, VALID_PAYLOAD, HMAC_SHA_256); + private static final JSONObject VALID_BODY = new JSONObject().put("payload", VALID_PAYLOAD).put("hmac", VALID_HMAC); @Test public void notLoggedIn_Returns401() throws Exception { @@ -62,18 +68,64 @@ public void missingPayload_Returns400() throws Exception { @Test public void numericPayload_Returns200() throws Exception { var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var response = client.post(validUrl(), new JSONObject().put("payload", 123).toString()); + var body = new JSONObject().put("payload", 123).put("hmac", sign(HMAC_SECRET, "123", HMAC_SHA_256)); + var response = client.post(validUrl(), body); response.readEntity(String.class); } @Test public void extraAttribute_Returns400() throws Exception { var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var response = client.post(validUrl(), new JSONObject().put("payload", "some_signed_payload").put("extra", "value").toString()); + var body = new JSONObject().put("payload", VALID_PAYLOAD).put("hmac", VALID_HMAC).put("extra", "value"); + var response = client.post(validUrl(), body); response.assertError("Invalid JSON object submitted", Response.Status.BAD_REQUEST); } } + @Nested + class HmacVerification { + @Test + public void missingHmac_Returns400() throws Exception { + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); + var response = client.post(validUrl(), new JSONObject().put("payload", VALID_PAYLOAD)); + response.assertError("Invalid JSON object submitted", Response.Status.BAD_REQUEST); + } + + @Test + public void invalidHmac_Returns400() throws Exception { + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); + var body = new JSONObject().put("payload", VALID_PAYLOAD).put("hmac", "not-a-valid-signature"); + var response = client.post(validUrl(), body); + response.assertError("Invalid HMAC signature", Response.Status.BAD_REQUEST); + } + + @Test + public void wrongSecret_Returns400() throws Exception { + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); + var body = new JSONObject().put("payload", VALID_PAYLOAD) + .put("hmac", sign("wrong-secret", VALID_PAYLOAD, HMAC_SHA_256)); + var response = client.post(validUrl(), body); + response.assertError("Invalid HMAC signature", Response.Status.BAD_REQUEST); + } + + @Test + public void tamperedPayload_Returns400() throws Exception { + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); + var body = new JSONObject().put("payload", "tampered_payload").put("hmac", VALID_HMAC); + var response = client.post(validUrl(), body); + response.assertError("Invalid HMAC signature", Response.Status.BAD_REQUEST); + } + + @Test + public void wrongAlgo_Returns400() throws Exception { + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); + var body = new JSONObject().put("payload", VALID_PAYLOAD) + .put("hmac", sign(HMAC_SECRET, VALID_PAYLOAD, "HmacMD5")); + var response = client.post(validUrl(), body); + response.assertError("Invalid HMAC signature", Response.Status.BAD_REQUEST); + } + } + @Test public void happy_happy() throws Exception { var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); @@ -81,7 +133,7 @@ public void happy_happy() throws Exception { response.readEntity(String.class); } - private GitContentManager brokenContentManager() throws UnknownHostException { + private GitContentManager brokenContentManager() throws Exception { var brokenClient = ElasticSearchProvider.getClient("localhost", 1, "elastic", "elastic"); var brokenProvider = new ElasticSearchProvider(brokenClient); return new GitContentManager(null, brokenProvider, mainMapper, contentMapper, properties); @@ -92,6 +144,18 @@ private String validUrl() throws Exception { return "/skills/" + app.getString("id") + "/answer"; } + private static String sign(final String key, final String payload, final String algo) { + try { + SecretKeySpec signingKey = new SecretKeySpec(key.getBytes(), algo); + Mac mac = Mac.getInstance(algo); + mac.init(signingKey); + + return new String(Base64.encodeBase64(mac.doFinal(payload.getBytes()))); + } catch (final Exception e) { + return "NOT_SIGNED"; + } + } + private TestServer testServer() throws Exception { return testServer(contentManager); } @@ -99,7 +163,7 @@ private TestServer testServer() throws Exception { private TestServer testServer(final GitContentManager cm) throws Exception { return startServer( new AuthenticationFacade(properties, userAccountManager, logManager, misuseMonitor), - new SkillsFacade(properties, userAccountManager, logManager, cm, new SkillsManager()) + new SkillsFacade(properties, userAccountManager, logManager, cm, new SkillsManager(properties)) ); } } diff --git a/src/test/resources/segue-integration-test-config.yaml b/src/test/resources/segue-integration-test-config.yaml index 45ae8836e9..e2b2d1fa66 100644 --- a/src/test/resources/segue-integration-test-config.yaml +++ b/src/test/resources/segue-integration-test-config.yaml @@ -58,6 +58,7 @@ MAX_CONCURRENT_WEB_SOCKETS_PER_USER: "10" FASTTRACK_GAMEBOARD_WHITELIST: ft_core_2017,ft_core_2018 # Security HMAC_SALT: 0982374959082374098572930SUPER-SECRET-KEY5873246586857342867 +SKILLS_HMAC_SECRET: integration-test-anvil-hmac-secret SESSION_EXPIRY_SECONDS_DEFAULT: "432000" SESSION_EXPIRY_SECONDS_REMEMBERED: "1209600" EMAIL_VERIFICATION_ENDPOINT_TOKEN: some-very-secret-token From c8d4df2dd4352c0be478d422a50926071136c610 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Mon, 15 Jun 2026 15:02:20 +0100 Subject: [PATCH 10/31] validate payload contents --- .../ac/cam/cl/dtg/isaac/api/SkillsFacade.java | 3 +- .../dtg/isaac/api/managers/SkillsManager.java | 25 +++++++ .../cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java | 46 +++++++++++++ .../cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 68 ++++++++++++++++++- 4 files changed, 138 insertions(+), 4 deletions(-) create mode 100644 src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java index e29092a3c0..8f7bd7856b 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java @@ -65,11 +65,12 @@ public Response answerQuestion(@Context final HttpServletRequest request, @PathParam("appId") final String appId, final String body) { try { - userManager.getCurrentRegisteredUser(request); + var currentUser = userManager.getCurrentRegisteredUser(request); var markingResponse = skillsManager.parseResponse(body); if (!skillsManager.isHmacValid(markingResponse)) { return new SegueErrorResponse(Status.BAD_REQUEST, "Invalid HMAC signature").toResponse(); } + skillsManager.validatePayload(markingResponse.getPayload(), currentUser.getId()); if (!(this.contentManager.getContentDOById(appId) instanceof AnvilApp)) { var error = new SegueErrorResponse(Status.NOT_FOUND, "No app found for given id: " + appId); log.warn(error.getErrorMessage()); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java index 8a12fdd9e1..cef9ffe507 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java @@ -4,9 +4,12 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.inject.Inject; import uk.ac.cam.cl.dtg.isaac.dto.AnvilMarkingResponseDTO; +import uk.ac.cam.cl.dtg.isaac.dto.AnvilPayloadDTO; import uk.ac.cam.cl.dtg.segue.api.managers.UserAuthenticationManager; import uk.ac.cam.cl.dtg.util.AbstractConfigLoader; +import java.util.Date; + import static uk.ac.cam.cl.dtg.segue.api.Constants.*; @@ -53,4 +56,26 @@ public boolean isHmacValid(final AnvilMarkingResponseDTO dto) { String expected = UserAuthenticationManager.calculateHMAC(hmacSecret, dto.getPayload()); return expected.equals(dto.getHmac()); } + + /** + * Validates the content of the signed payload string. + * + * @param payloadStr - the payload string from the marking response + * @param userId - the ID of the currently authenticated user + * @throws InvalidMarkingResponseException if the payload is malformed, the user ID does not match, + * or the timestamp is outside the allowed window + */ + public void validatePayload(final String payloadStr, final long userId) throws InvalidMarkingResponseException { + try { + AnvilPayloadDTO dto = objectMapper.readValue(payloadStr, AnvilPayloadDTO.class); + if (dto.getUserId() != userId) { + throw new InvalidMarkingResponseException("Payload user_id does not match session"); + } + if (dto.getTimestamp().before(new Date(System.currentTimeMillis() - 300_000L))) { + throw new InvalidMarkingResponseException("Payload timestamp is outside the allowed window"); + } + } catch (JsonProcessingException e) { + throw new InvalidMarkingResponseException("Invalid payload"); + } + } } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java new file mode 100644 index 0000000000..d99d581d91 --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java @@ -0,0 +1,46 @@ +package uk.ac.cam.cl.dtg.isaac.dto; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; + +import java.util.Date; + +/** + * DTO representing the signed payload content from the external Anvil marking server. + */ +public class AnvilPayloadDTO { + private final long userId; + private final Date timestamp; + + /** + * Constructor. + * + * @param userId - the ID of the user who answered the question + * @param timestamp - the datetime at which the response was signed + */ + @JsonCreator + public AnvilPayloadDTO( + @JsonProperty(value = "user_id", required = true) final long userId, + @JsonProperty(value = "timestamp", required = true) final Date timestamp) { + this.userId = userId; + this.timestamp = timestamp; + } + + /** + * Gets the user ID. + * + * @return the user ID + */ + public long getUserId() { + return userId; + } + + /** + * Gets the timestamp. + * + * @return the signing timestamp + */ + public Date getTimestamp() { + return timestamp; + } +} diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index 01d6868493..7b9f0018a7 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -13,13 +13,19 @@ import javax.crypto.Mac; import javax.crypto.spec.SecretKeySpec; +import java.time.Instant; + import static uk.ac.cam.cl.dtg.isaac.api.ITConstants.REGRESSION_TEST_PAGE_ID; +import static uk.ac.cam.cl.dtg.isaac.api.ITConstants.TEST_STUDENT_ID; @SuppressWarnings("checkstyle:MissingJavadocType") public class SkillsFacadeIT extends IsaacIntegrationTestWithREST { private static final String HMAC_SECRET = "integration-test-anvil-hmac-secret"; - private static final String VALID_PAYLOAD = "some_signed_payload"; private static final String HMAC_SHA_256 = "HmacSHA256"; + private static final String VALID_PAYLOAD = new JSONObject() + .put("user_id", TEST_STUDENT_ID) + .put("timestamp", Instant.now()) + .toString(); private static final String VALID_HMAC = sign(HMAC_SECRET, VALID_PAYLOAD, HMAC_SHA_256); private static final JSONObject VALID_BODY = new JSONObject().put("payload", VALID_PAYLOAD).put("hmac", VALID_HMAC); @@ -66,11 +72,11 @@ public void missingPayload_Returns400() throws Exception { } @Test - public void numericPayload_Returns200() throws Exception { + public void numericPayload_Returns400() throws Exception { var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); var body = new JSONObject().put("payload", 123).put("hmac", sign(HMAC_SECRET, "123", HMAC_SHA_256)); var response = client.post(validUrl(), body); - response.readEntity(String.class); + response.assertError("Invalid payload", Response.Status.BAD_REQUEST); } @Test @@ -126,6 +132,57 @@ public void wrongAlgo_Returns400() throws Exception { } } + @Nested + class PayloadContentCheck { + @Test + public void missingUserId_Returns400() throws Exception { + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); + var payload = new JSONObject().put("timestamp", Instant.now()); + var response = client.post(validUrl(), wrapSigned(payload)); + response.assertError("Invalid payload", Response.Status.BAD_REQUEST); + } + + @Test + public void nonNumericUserId_Returns400() throws Exception { + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); + var payload = new JSONObject().put("user_id", "not-a-number").put("timestamp", Instant.now()); + var response = client.post(validUrl(), wrapSigned(payload)); + response.assertError("Invalid payload", Response.Status.BAD_REQUEST); + } + + @Test + public void missingTimestamp_Returns400() throws Exception { + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); + var payload = new JSONObject().put("user_id", TEST_STUDENT_ID); + var response = client.post(validUrl(), wrapSigned(payload)); + response.assertError("Invalid payload", Response.Status.BAD_REQUEST); + } + + @Test + public void invalidTimestamp_Returns400() throws Exception { + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); + var payload = new JSONObject().put("user_id", TEST_STUDENT_ID).put("timestamp", "not-a-date"); + var response = client.post(validUrl(), wrapSigned(payload)); + response.assertError("Invalid payload", Response.Status.BAD_REQUEST); + } + + @Test + public void wrongUserId_Returns400() throws Exception { + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); + var payload = new JSONObject().put("user_id", TEST_STUDENT_ID + 1).put("timestamp", Instant.now()); + var response = client.post(validUrl(), wrapSigned(payload)); + response.assertError("Payload user_id does not match session", Response.Status.BAD_REQUEST); + } + + @Test + public void expiredTimestamp_Returns400() throws Exception { + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); + var payload = new JSONObject().put("user_id", TEST_STUDENT_ID).put("timestamp", "2022-01-01T13:00:00Z"); + var response = client.post(validUrl(), wrapSigned(payload)); + response.assertError("Payload timestamp is outside the allowed window", Response.Status.BAD_REQUEST); + } + } + @Test public void happy_happy() throws Exception { var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); @@ -144,6 +201,11 @@ private String validUrl() throws Exception { return "/skills/" + app.getString("id") + "/answer"; } + private JSONObject wrapSigned(JSONObject payload) { + String data = payload.toString(); + return new JSONObject().put("payload", data).put("hmac", sign(HMAC_SECRET, data, HMAC_SHA_256)); + } + private static String sign(final String key, final String payload, final String algo) { try { SecretKeySpec signingKey = new SecretKeySpec(key.getBytes(), algo); From ab2cd294dbb4a6d11361abe688ee568a3ebd37f5 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Mon, 15 Jun 2026 15:03:44 +0100 Subject: [PATCH 11/31] fix checkstyle --- .../uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java | 2 +- src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java index cef9ffe507..9fb4dee475 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java @@ -74,7 +74,7 @@ public void validatePayload(final String payloadStr, final long userId) throws I if (dto.getTimestamp().before(new Date(System.currentTimeMillis() - 300_000L))) { throw new InvalidMarkingResponseException("Payload timestamp is outside the allowed window"); } - } catch (JsonProcessingException e) { + } catch (final JsonProcessingException e) { throw new InvalidMarkingResponseException("Invalid payload"); } } diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index 7b9f0018a7..f4c1668996 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -10,11 +10,10 @@ import uk.ac.cam.cl.dtg.segue.search.ElasticSearchProvider; import jakarta.ws.rs.core.Response; +import java.time.Instant; import javax.crypto.Mac; import javax.crypto.spec.SecretKeySpec; -import java.time.Instant; - import static uk.ac.cam.cl.dtg.isaac.api.ITConstants.REGRESSION_TEST_PAGE_ID; import static uk.ac.cam.cl.dtg.isaac.api.ITConstants.TEST_STUDENT_ID; @@ -201,7 +200,7 @@ private String validUrl() throws Exception { return "/skills/" + app.getString("id") + "/answer"; } - private JSONObject wrapSigned(JSONObject payload) { + private JSONObject wrapSigned(final JSONObject payload) { String data = payload.toString(); return new JSONObject().put("payload", data).put("hmac", sign(HMAC_SECRET, data, HMAC_SHA_256)); } From f79f4e56da7388712a206052ae36082856f80771 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Mon, 15 Jun 2026 15:40:53 +0100 Subject: [PATCH 12/31] use data provider for tests --- .../cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 149 +++++++----------- 1 file changed, 59 insertions(+), 90 deletions(-) diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index f4c1668996..e3fb5456ae 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -4,6 +4,10 @@ import org.json.JSONObject; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import uk.ac.cam.cl.dtg.isaac.api.managers.SkillsManager; import uk.ac.cam.cl.dtg.segue.api.AuthenticationFacade; import uk.ac.cam.cl.dtg.segue.dao.content.GitContentManager; @@ -11,6 +15,8 @@ import jakarta.ws.rs.core.Response; import java.time.Instant; +import java.util.function.Consumer; +import java.util.stream.Stream; import javax.crypto.Mac; import javax.crypto.spec.SecretKeySpec; @@ -21,10 +27,7 @@ public class SkillsFacadeIT extends IsaacIntegrationTestWithREST { private static final String HMAC_SECRET = "integration-test-anvil-hmac-secret"; private static final String HMAC_SHA_256 = "HmacSHA256"; - private static final String VALID_PAYLOAD = new JSONObject() - .put("user_id", TEST_STUDENT_ID) - .put("timestamp", Instant.now()) - .toString(); + private static final String VALID_PAYLOAD = validPayload(p -> {}); private static final String VALID_HMAC = sign(HMAC_SECRET, VALID_PAYLOAD, HMAC_SHA_256); private static final JSONObject VALID_BODY = new JSONObject().put("payload", VALID_PAYLOAD).put("hmac", VALID_HMAC); @@ -88,97 +91,60 @@ public void extraAttribute_Returns400() throws Exception { } @Nested + @TestInstance(TestInstance.Lifecycle.PER_CLASS) class HmacVerification { - @Test - public void missingHmac_Returns400() throws Exception { - var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var response = client.post(validUrl(), new JSONObject().put("payload", VALID_PAYLOAD)); - response.assertError("Invalid JSON object submitted", Response.Status.BAD_REQUEST); - } - - @Test - public void invalidHmac_Returns400() throws Exception { - var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var body = new JSONObject().put("payload", VALID_PAYLOAD).put("hmac", "not-a-valid-signature"); - var response = client.post(validUrl(), body); - response.assertError("Invalid HMAC signature", Response.Status.BAD_REQUEST); - } - - @Test - public void wrongSecret_Returns400() throws Exception { - var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var body = new JSONObject().put("payload", VALID_PAYLOAD) - .put("hmac", sign("wrong-secret", VALID_PAYLOAD, HMAC_SHA_256)); - var response = client.post(validUrl(), body); - response.assertError("Invalid HMAC signature", Response.Status.BAD_REQUEST); - } - - @Test - public void tamperedPayload_Returns400() throws Exception { - var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var body = new JSONObject().put("payload", "tampered_payload").put("hmac", VALID_HMAC); - var response = client.post(validUrl(), body); - response.assertError("Invalid HMAC signature", Response.Status.BAD_REQUEST); - } - - @Test - public void wrongAlgo_Returns400() throws Exception { + Stream invalidBodies() { + return Stream.of( + Arguments.of("missing hmac", new JSONObject().put("payload", VALID_PAYLOAD), + "Invalid JSON object submitted"), + Arguments.of("invalid hmac", new JSONObject().put("payload", VALID_PAYLOAD) + .put("hmac", "not-a-valid-signature"), "Invalid HMAC signature"), + Arguments.of("wrong secret", new JSONObject().put("payload", VALID_PAYLOAD) + .put("hmac", sign("wrong-secret", VALID_PAYLOAD, HMAC_SHA_256)), "Invalid HMAC signature"), + Arguments.of("tampered payload", new JSONObject().put("payload", "tampered_payload") + .put("hmac", VALID_HMAC), "Invalid HMAC signature"), + Arguments.of("wrong algorithm", new JSONObject().put("payload", VALID_PAYLOAD) + .put("hmac", sign(HMAC_SECRET, VALID_PAYLOAD, "HmacMD5")), "Invalid HMAC signature") + ); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("invalidBodies") + public void invalidBody_Returns400( + final String name, final JSONObject body, final String expectedMessage + ) throws Exception { var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var body = new JSONObject().put("payload", VALID_PAYLOAD) - .put("hmac", sign(HMAC_SECRET, VALID_PAYLOAD, "HmacMD5")); - var response = client.post(validUrl(), body); - response.assertError("Invalid HMAC signature", Response.Status.BAD_REQUEST); + client.post(validUrl(), body).assertError(expectedMessage, Response.Status.BAD_REQUEST); } } @Nested + @TestInstance(TestInstance.Lifecycle.PER_CLASS) class PayloadContentCheck { - @Test - public void missingUserId_Returns400() throws Exception { - var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var payload = new JSONObject().put("timestamp", Instant.now()); - var response = client.post(validUrl(), wrapSigned(payload)); - response.assertError("Invalid payload", Response.Status.BAD_REQUEST); - } - - @Test - public void nonNumericUserId_Returns400() throws Exception { - var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var payload = new JSONObject().put("user_id", "not-a-number").put("timestamp", Instant.now()); - var response = client.post(validUrl(), wrapSigned(payload)); - response.assertError("Invalid payload", Response.Status.BAD_REQUEST); - } - - @Test - public void missingTimestamp_Returns400() throws Exception { - var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var payload = new JSONObject().put("user_id", TEST_STUDENT_ID); - var response = client.post(validUrl(), wrapSigned(payload)); - response.assertError("Invalid payload", Response.Status.BAD_REQUEST); - } - - @Test - public void invalidTimestamp_Returns400() throws Exception { - var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var payload = new JSONObject().put("user_id", TEST_STUDENT_ID).put("timestamp", "not-a-date"); - var response = client.post(validUrl(), wrapSigned(payload)); - response.assertError("Invalid payload", Response.Status.BAD_REQUEST); - } - - @Test - public void wrongUserId_Returns400() throws Exception { - var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var payload = new JSONObject().put("user_id", TEST_STUDENT_ID + 1).put("timestamp", Instant.now()); - var response = client.post(validUrl(), wrapSigned(payload)); - response.assertError("Payload user_id does not match session", Response.Status.BAD_REQUEST); - } - - @Test - public void expiredTimestamp_Returns400() throws Exception { + Stream invalidPayloads() { + return Stream.of( + Arguments.of("missing user_id", validPayload(p -> p.remove("user_id")), "Invalid payload"), + Arguments.of("non-numeric user_id", validPayload(p -> p.put("user_id", "ab")), "Invalid payload"), + Arguments.of("missing timestamp", validPayload(p -> p.remove("timestamp")), "Invalid payload"), + Arguments.of("invalid timestamp", validPayload(p -> p.put("timestamp", "not-a-date")), + "Invalid payload"), + Arguments.of("wrong user_id", validPayload(p -> p.put("user_id", TEST_STUDENT_ID + 1)), + "Payload user_id does not match session"), + Arguments.of("expired timestamp", validPayload(p -> p.put("timestamp", "2022-01-01T13:00:00Z")), + "Payload timestamp is outside the allowed window") + ); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("invalidPayloads") + public void invalidPayload_Returns400( + final String name, final JSONObject payload, final String expectedMessage + ) throws Exception { var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var payload = new JSONObject().put("user_id", TEST_STUDENT_ID).put("timestamp", "2022-01-01T13:00:00Z"); - var response = client.post(validUrl(), wrapSigned(payload)); - response.assertError("Payload timestamp is outside the allowed window", Response.Status.BAD_REQUEST); + var body = new JSONObject() + .put("payload", payload.toString()) + .put("hmac", sign(HMAC_SECRET, payload.toString(), HMAC_SHA_256)); + client.post(validUrl(), body).assertError(expectedMessage, Response.Status.BAD_REQUEST); } } @@ -200,9 +166,12 @@ private String validUrl() throws Exception { return "/skills/" + app.getString("id") + "/answer"; } - private JSONObject wrapSigned(final JSONObject payload) { - String data = payload.toString(); - return new JSONObject().put("payload", data).put("hmac", sign(HMAC_SECRET, data, HMAC_SHA_256)); + private static String validPayload(final Consumer ops) { + var defaultValidPayload = new JSONObject() + .put("user_id", TEST_STUDENT_ID) + .put("timestamp", Instant.now()); + ops.accept(defaultValidPayload); + return defaultValidPayload.toString(); } private static String sign(final String key, final String payload, final String algo) { From c3444f231bb0aea1d5ec322eb4287bf3494bee84 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Mon, 15 Jun 2026 15:48:54 +0100 Subject: [PATCH 13/31] minor refactor --- .../ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index e3fb5456ae..4a649432e9 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -113,8 +113,9 @@ Stream invalidBodies() { public void invalidBody_Returns400( final String name, final JSONObject body, final String expectedMessage ) throws Exception { - var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - client.post(validUrl(), body).assertError(expectedMessage, Response.Status.BAD_REQUEST); + testServer().client().loginAs(integrationTestUsers.TEST_STUDENT) + .post(validUrl(), body) + .assertError(expectedMessage, Response.Status.BAD_REQUEST); } } @@ -140,11 +141,11 @@ Stream invalidPayloads() { public void invalidPayload_Returns400( final String name, final JSONObject payload, final String expectedMessage ) throws Exception { - var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var body = new JSONObject() - .put("payload", payload.toString()) - .put("hmac", sign(HMAC_SECRET, payload.toString(), HMAC_SHA_256)); - client.post(validUrl(), body).assertError(expectedMessage, Response.Status.BAD_REQUEST); + testServer().client().loginAs(integrationTestUsers.TEST_STUDENT) + .post(validUrl(), new JSONObject() + .put("payload", payload.toString()) + .put("hmac", sign(HMAC_SECRET, payload.toString(), HMAC_SHA_256)) + ).assertError(expectedMessage, Response.Status.BAD_REQUEST); } } From 35421fcccb2e152113f0252b3744366ace42db91 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Mon, 15 Jun 2026 16:33:59 +0100 Subject: [PATCH 14/31] simplify a few test cases --- .../ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index 4a649432e9..57d19bff71 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -4,7 +4,6 @@ import org.json.JSONObject; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.TestInstance; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; @@ -41,24 +40,21 @@ public void notLoggedIn_Returns401() throws Exception { class AppIdCheck { @Test public void elasticsearchUnavailable_Returns404() throws Exception { - var client = testServer(brokenContentManager()).client(); - client.loginAs(integrationTestUsers.TEST_STUDENT); + var client = testServer(brokenContentManager()).client().loginAs(integrationTestUsers.TEST_STUDENT); var response = client.post("/skills/unknown_app/answer", VALID_BODY); response.assertError("Error locating the version requested", Response.Status.NOT_FOUND); } @Test public void unknownApp_Returns404() throws Exception { - var client = testServer().client(); - client.loginAs(integrationTestUsers.TEST_STUDENT); + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); var response = client.post("/skills/unknown_app/answer", VALID_BODY); response.assertError("No app found for given id: unknown_app", Response.Status.NOT_FOUND); } @Test public void idMatchesNonApp_Returns404() throws Exception { - var client = testServer().client(); - client.loginAs(integrationTestUsers.TEST_STUDENT); + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); var response = client.post("/skills/" + REGRESSION_TEST_PAGE_ID + "/answer", VALID_BODY); response.assertError("No app found for given id: " + REGRESSION_TEST_PAGE_ID, Response.Status.NOT_FOUND); } @@ -91,9 +87,8 @@ public void extraAttribute_Returns400() throws Exception { } @Nested - @TestInstance(TestInstance.Lifecycle.PER_CLASS) class HmacVerification { - Stream invalidBodies() { + static Stream invalidBodies() { return Stream.of( Arguments.of("missing hmac", new JSONObject().put("payload", VALID_PAYLOAD), "Invalid JSON object submitted"), @@ -120,9 +115,8 @@ public void invalidBody_Returns400( } @Nested - @TestInstance(TestInstance.Lifecycle.PER_CLASS) class PayloadContentCheck { - Stream invalidPayloads() { + static Stream invalidPayloads() { return Stream.of( Arguments.of("missing user_id", validPayload(p -> p.remove("user_id")), "Invalid payload"), Arguments.of("non-numeric user_id", validPayload(p -> p.put("user_id", "ab")), "Invalid payload"), From 7890d3923d2d9a00c5e185ffff60ee51cac24a88 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Mon, 15 Jun 2026 23:41:22 +0100 Subject: [PATCH 15/31] actually record answers --- .../cl/dtg/isaac/ISkillsAttemptManager.java | 7 +++++ .../ac/cam/cl/dtg/isaac/api/SkillsFacade.java | 18 +++++++---- .../dtg/isaac/api/managers/SkillsManager.java | 22 +++++++++++-- .../dtg/isaac/dao/PgSkillsAttemptManager.java | 31 +++++++++++++++++++ .../2026-06-skills_question_attempts.sql | 6 ++++ .../postgres-rutherford-create-script.sql | 10 ++++++ .../cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 24 +++++++++++++- 7 files changed, 108 insertions(+), 10 deletions(-) create mode 100644 src/main/java/uk/ac/cam/cl/dtg/isaac/ISkillsAttemptManager.java create mode 100644 src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java create mode 100644 src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/ISkillsAttemptManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/ISkillsAttemptManager.java new file mode 100644 index 0000000000..dcda860f05 --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/ISkillsAttemptManager.java @@ -0,0 +1,7 @@ +package uk.ac.cam.cl.dtg.isaac; + +import uk.ac.cam.cl.dtg.isaac.dto.AnvilPayloadDTO; + +public interface ISkillsAttemptManager { + void registerSkillsAttempt(final AnvilPayloadDTO attempt); +} diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java index 8f7bd7856b..65271fe6eb 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java @@ -7,7 +7,9 @@ import uk.ac.cam.cl.dtg.isaac.api.managers.InvalidMarkingResponseException; import uk.ac.cam.cl.dtg.isaac.api.managers.SkillsManager; import uk.ac.cam.cl.dtg.isaac.dos.content.AnvilApp; +import uk.ac.cam.cl.dtg.isaac.dto.AnvilMarkingResponseDTO; import uk.ac.cam.cl.dtg.isaac.dto.SegueErrorResponse; +import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; import uk.ac.cam.cl.dtg.segue.api.managers.UserAccountManager; import uk.ac.cam.cl.dtg.segue.auth.exceptions.NoUserLoggedInException; import uk.ac.cam.cl.dtg.segue.dao.ILogManager; @@ -65,17 +67,21 @@ public Response answerQuestion(@Context final HttpServletRequest request, @PathParam("appId") final String appId, final String body) { try { - var currentUser = userManager.getCurrentRegisteredUser(request); - var markingResponse = skillsManager.parseResponse(body); - if (!skillsManager.isHmacValid(markingResponse)) { - return new SegueErrorResponse(Status.BAD_REQUEST, "Invalid HMAC signature").toResponse(); - } - skillsManager.validatePayload(markingResponse.getPayload(), currentUser.getId()); + RegisteredUserDTO currentUser = userManager.getCurrentRegisteredUser(request); if (!(this.contentManager.getContentDOById(appId) instanceof AnvilApp)) { var error = new SegueErrorResponse(Status.NOT_FOUND, "No app found for given id: " + appId); log.warn(error.getErrorMessage()); return error.toResponse(); } + + AnvilMarkingResponseDTO markingResponse = skillsManager.parseResponse(body); + if (!skillsManager.isHmacValid(markingResponse)) { + return new SegueErrorResponse(Status.BAD_REQUEST, "Invalid HMAC signature").toResponse(); + } + + var payloadDTO = skillsManager.parsePayload(markingResponse.getPayload(), currentUser.getId()); + skillsManager.recordAttempt(payloadDTO); + return Response.ok().build(); } catch (final NoUserLoggedInException e) { return SegueErrorResponse.getNotLoggedInResponse(); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java index 9fb4dee475..78f179fb8f 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java @@ -3,6 +3,7 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.inject.Inject; +import uk.ac.cam.cl.dtg.isaac.ISkillsAttemptManager; import uk.ac.cam.cl.dtg.isaac.dto.AnvilMarkingResponseDTO; import uk.ac.cam.cl.dtg.isaac.dto.AnvilPayloadDTO; import uk.ac.cam.cl.dtg.segue.api.managers.UserAuthenticationManager; @@ -20,15 +21,18 @@ public class SkillsManager { private static final ObjectMapper objectMapper = new ObjectMapper(); private final String hmacSecret; + private final ISkillsAttemptManager skillsAttemptManager; /** * Constructor. * - * @param properties - application configuration + * @param properties - application configuration + * @param skillsAttemptManager - persistence manager for skills attempts */ @Inject - public SkillsManager(final AbstractConfigLoader properties) { + public SkillsManager(final AbstractConfigLoader properties, final ISkillsAttemptManager skillsAttemptManager) { this.hmacSecret = properties.getProperty(SKILLS_HMAC_SECRET); + this.skillsAttemptManager = skillsAttemptManager; } /** @@ -65,7 +69,9 @@ public boolean isHmacValid(final AnvilMarkingResponseDTO dto) { * @throws InvalidMarkingResponseException if the payload is malformed, the user ID does not match, * or the timestamp is outside the allowed window */ - public void validatePayload(final String payloadStr, final long userId) throws InvalidMarkingResponseException { + public AnvilPayloadDTO parsePayload( + final String payloadStr, final long userId + ) throws InvalidMarkingResponseException { try { AnvilPayloadDTO dto = objectMapper.readValue(payloadStr, AnvilPayloadDTO.class); if (dto.getUserId() != userId) { @@ -74,8 +80,18 @@ public void validatePayload(final String payloadStr, final long userId) throws I if (dto.getTimestamp().before(new Date(System.currentTimeMillis() - 300_000L))) { throw new InvalidMarkingResponseException("Payload timestamp is outside the allowed window"); } + return dto; } catch (final JsonProcessingException e) { throw new InvalidMarkingResponseException("Invalid payload"); } } + + /** + * Records a validated skills attempt. + * + * @param attempt - the validated payload DTO + */ + public void recordAttempt(final AnvilPayloadDTO attempt) { + skillsAttemptManager.registerSkillsAttempt(attempt); + } } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java new file mode 100644 index 0000000000..0a1471f051 --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java @@ -0,0 +1,31 @@ +package uk.ac.cam.cl.dtg.isaac.dao; + +import com.google.inject.Inject; +import uk.ac.cam.cl.dtg.isaac.ISkillsAttemptManager; +import uk.ac.cam.cl.dtg.isaac.dto.AnvilPayloadDTO; +import uk.ac.cam.cl.dtg.segue.database.PostgresSqlDb; + +import java.sql.SQLException; +import java.sql.Timestamp; + +public class PgSkillsAttemptManager implements ISkillsAttemptManager { + private final PostgresSqlDb database; + + @Inject + public PgSkillsAttemptManager(final PostgresSqlDb database) { + this.database = database; + } + + @Override + public void registerSkillsAttempt(final AnvilPayloadDTO attempt) { + try (var conn = database.getDatabaseConnection(); + var pst = conn.prepareStatement( + "INSERT INTO skills_question_attempts (user_id, timestamp) VALUES (?, ?)")) { + pst.setLong(1, attempt.getUserId()); + pst.setTimestamp(2, new Timestamp(attempt.getTimestamp().getTime())); + pst.executeUpdate(); + } catch (final SQLException e) { + throw new RuntimeException("Failed to record skills attempt", e); + } + } +} diff --git a/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql b/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql new file mode 100644 index 0000000000..39b71ed88e --- /dev/null +++ b/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql @@ -0,0 +1,6 @@ +CREATE TABLE public.skills_question_attempts ( + user_id INTEGER NOT NULL, + timestamp TIMESTAMP WITH TIME ZONE NOT NULL +); + +ALTER TABLE public.skills_question_attempts OWNER to rutherford; diff --git a/src/main/resources/db_scripts/postgres-rutherford-create-script.sql b/src/main/resources/db_scripts/postgres-rutherford-create-script.sql index db99928ea9..6f92e2c68a 100644 --- a/src/main/resources/db_scripts/postgres-rutherford-create-script.sql +++ b/src/main/resources/db_scripts/postgres-rutherford-create-script.sql @@ -599,6 +599,16 @@ CREATE TABLE public.user_bookmarks ( ALTER TABLE public.user_bookmarks OWNER TO rutherford; +-- +-- Name: skill_question_attempts; Type: TABLE; Schema: public; Owner: rutherford +-- +CREATE TABLE public.skills_question_attempts ( + user_id INTEGER NOT NULL, + timestamp TIMESTAMP WITH TIME ZONE NOT NULL +); + +ALTER TABLE public.skills_question_attempts OWNER to rutherford; + -- -- Name: user_credentials; Type: TABLE; Schema: public; Owner: rutherford -- diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index 57d19bff71..4acb7b45e2 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -8,17 +8,22 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import uk.ac.cam.cl.dtg.isaac.api.managers.SkillsManager; +import uk.ac.cam.cl.dtg.isaac.dao.PgSkillsAttemptManager; import uk.ac.cam.cl.dtg.segue.api.AuthenticationFacade; import uk.ac.cam.cl.dtg.segue.dao.content.GitContentManager; import uk.ac.cam.cl.dtg.segue.search.ElasticSearchProvider; import jakarta.ws.rs.core.Response; import java.time.Instant; +import java.time.temporal.ChronoUnit; import java.util.function.Consumer; import java.util.stream.Stream; import javax.crypto.Mac; import javax.crypto.spec.SecretKeySpec; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.within; +import static org.junit.jupiter.api.Assertions.assertEquals; import static uk.ac.cam.cl.dtg.isaac.api.ITConstants.REGRESSION_TEST_PAGE_ID; import static uk.ac.cam.cl.dtg.isaac.api.ITConstants.TEST_STUDENT_ID; @@ -148,6 +153,22 @@ public void happy_happy() throws Exception { var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); var response = client.post(validUrl(), VALID_BODY); response.readEntity(String.class); + + try (var conn = postgresSqlDb.getDatabaseConnection(); + var pst = conn.prepareStatement("SELECT COUNT(*) FROM public.skills_question_attempts"); + var rs = pst.executeQuery()) { + rs.next(); + assertEquals(1, rs.getInt(1)); + } + + try (var conn = postgresSqlDb.getDatabaseConnection(); + var pst = conn.prepareStatement("SELECT user_id, timestamp FROM public.skills_question_attempts"); + var rs = pst.executeQuery()) { + rs.next(); + assertEquals(TEST_STUDENT_ID, rs.getInt("user_id")); + assertThat(rs.getTimestamp("timestamp").toInstant()) + .isCloseTo(new JSONObject(VALID_PAYLOAD).getString("timestamp"), within(1, ChronoUnit.MILLIS)); + } } private GitContentManager brokenContentManager() throws Exception { @@ -186,9 +207,10 @@ private TestServer testServer() throws Exception { } private TestServer testServer(final GitContentManager cm) throws Exception { + var sm = new SkillsManager(properties, new PgSkillsAttemptManager(postgresSqlDb)); return startServer( new AuthenticationFacade(properties, userAccountManager, logManager, misuseMonitor), - new SkillsFacade(properties, userAccountManager, logManager, cm, new SkillsManager(properties)) + new SkillsFacade(properties, userAccountManager, logManager, cm, sm) ); } } From d60bd9b2e3845b3878d2920e78ba3a4d9bc274dd Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Tue, 16 Jun 2026 11:48:39 +0100 Subject: [PATCH 16/31] minor changes --- .../cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index 4acb7b45e2..4bcd2f3e99 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -113,7 +113,8 @@ static Stream invalidBodies() { public void invalidBody_Returns400( final String name, final JSONObject body, final String expectedMessage ) throws Exception { - testServer().client().loginAs(integrationTestUsers.TEST_STUDENT) + testServer().client() + .loginAs(integrationTestUsers.TEST_STUDENT) .post(validUrl(), body) .assertError(expectedMessage, Response.Status.BAD_REQUEST); } @@ -140,11 +141,13 @@ static Stream invalidPayloads() { public void invalidPayload_Returns400( final String name, final JSONObject payload, final String expectedMessage ) throws Exception { - testServer().client().loginAs(integrationTestUsers.TEST_STUDENT) + testServer().client() + .loginAs(integrationTestUsers.TEST_STUDENT) .post(validUrl(), new JSONObject() .put("payload", payload.toString()) .put("hmac", sign(HMAC_SECRET, payload.toString(), HMAC_SHA_256)) - ).assertError(expectedMessage, Response.Status.BAD_REQUEST); + ) + .assertError(expectedMessage, Response.Status.BAD_REQUEST); } } @@ -165,9 +168,10 @@ public void happy_happy() throws Exception { var pst = conn.prepareStatement("SELECT user_id, timestamp FROM public.skills_question_attempts"); var rs = pst.executeQuery()) { rs.next(); - assertEquals(TEST_STUDENT_ID, rs.getInt("user_id")); + var jsonPayload = new JSONObject(VALID_PAYLOAD); + assertEquals(jsonPayload.getInt("user_id"), rs.getInt("user_id")); assertThat(rs.getTimestamp("timestamp").toInstant()) - .isCloseTo(new JSONObject(VALID_PAYLOAD).getString("timestamp"), within(1, ChronoUnit.MILLIS)); + .isCloseTo(jsonPayload.getString("timestamp"), within(1, ChronoUnit.MILLIS)); } } @@ -192,8 +196,8 @@ private static String validPayload(final Consumer ops) { private static String sign(final String key, final String payload, final String algo) { try { - SecretKeySpec signingKey = new SecretKeySpec(key.getBytes(), algo); - Mac mac = Mac.getInstance(algo); + var signingKey = new SecretKeySpec(key.getBytes(), algo); + var mac = Mac.getInstance(algo); mac.init(signingKey); return new String(Base64.encodeBase64(mac.doFinal(payload.getBytes()))); From 13aab683532e44f585d76847fe3d61aaab03631f Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Wed, 17 Jun 2026 10:14:15 +0100 Subject: [PATCH 17/31] WIP: validate all payload fields --- .../ac/cam/cl/dtg/isaac/api/SkillsFacade.java | 4 +- .../dtg/isaac/api/managers/SkillsManager.java | 17 ++- .../cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java | 66 ++++++--- .../api/IsaacIntegrationTestWithREST.java | 2 +- .../cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 136 +++++++++++++----- 5 files changed, 166 insertions(+), 59 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java index 65271fe6eb..6c1d126dee 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java @@ -74,12 +74,12 @@ public Response answerQuestion(@Context final HttpServletRequest request, return error.toResponse(); } - AnvilMarkingResponseDTO markingResponse = skillsManager.parseResponse(body); + AnvilMarkingResponseDTO markingResponse = skillsManager.parseRequest(body); if (!skillsManager.isHmacValid(markingResponse)) { return new SegueErrorResponse(Status.BAD_REQUEST, "Invalid HMAC signature").toResponse(); } - var payloadDTO = skillsManager.parsePayload(markingResponse.getPayload(), currentUser.getId()); + var payloadDTO = skillsManager.parsePayload(markingResponse.getPayload(), currentUser.getId(), appId); skillsManager.recordAttempt(payloadDTO); return Response.ok().build(); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java index 78f179fb8f..45491ad352 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java @@ -42,7 +42,7 @@ public SkillsManager(final AbstractConfigLoader properties, final ISkillsAttempt * @return the deserialised marking response * @throws InvalidMarkingResponseException if the body is missing or malformed */ - public AnvilMarkingResponseDTO parseResponse(final String body) throws InvalidMarkingResponseException { + public AnvilMarkingResponseDTO parseRequest(final String body) throws InvalidMarkingResponseException { try { return objectMapper.readValue(body, AnvilMarkingResponseDTO.class); } catch (final JsonProcessingException e) { @@ -66,20 +66,29 @@ public boolean isHmacValid(final AnvilMarkingResponseDTO dto) { * * @param payloadStr - the payload string from the marking response * @param userId - the ID of the currently authenticated user - * @throws InvalidMarkingResponseException if the payload is malformed, the user ID does not match, - * or the timestamp is outside the allowed window + * @param appId - the app ID from the URL, which must match the payload's skill_id + * @throws InvalidMarkingResponseException if the payload is malformed or any validation fails */ public AnvilPayloadDTO parsePayload( - final String payloadStr, final long userId + final String payloadStr, final long userId, final String appId ) throws InvalidMarkingResponseException { try { AnvilPayloadDTO dto = objectMapper.readValue(payloadStr, AnvilPayloadDTO.class); + if (dto.getSkillAssignmentId() != null) { + throw new InvalidMarkingResponseException("Invalid payload"); + } if (dto.getUserId() != userId) { throw new InvalidMarkingResponseException("Payload user_id does not match session"); } if (dto.getTimestamp().before(new Date(System.currentTimeMillis() - 300_000L))) { throw new InvalidMarkingResponseException("Payload timestamp is outside the allowed window"); } + if (!dto.getSkillId().equals(appId)) { + throw new InvalidMarkingResponseException("Payload skill_id does not match app"); + } + if (!(dto.getMarks() instanceof Integer n) || (n != 0 && n != 1)) { + throw new InvalidMarkingResponseException("Payload marks must be 0 or 1"); + } return dto; } catch (final JsonProcessingException e) { throw new InvalidMarkingResponseException("Invalid payload"); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java index d99d581d91..d3fa5489e5 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java @@ -2,45 +2,71 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonSetter; +import com.fasterxml.jackson.annotation.Nulls; import java.util.Date; +import java.util.UUID; /** * DTO representing the signed payload content from the external Anvil marking server. */ public class AnvilPayloadDTO { + private final UUID id; private final long userId; + private final String skillAssignmentId; + private final String skillId; + private final String subskillId; + private final Question question; + private final String questionAttempt; + private final Number marks; private final Date timestamp; /** * Constructor. * - * @param userId - the ID of the user who answered the question - * @param timestamp - the datetime at which the response was signed + * @param id - UUID identifying this attempt + * @param userId - the ID of the user who answered the question + * @param skillAssignmentId - the skill assignment ID (currently always null) + * @param skillId - the skill ID, must match the app ID from the URL + * @param subskillId - the subskill ID + * @param question - the question content + * @param questionAttempt - the student's answer + * @param marks - the marks awarded (0 or 1) + * @param timestamp - the datetime at which the response was signed */ @JsonCreator public AnvilPayloadDTO( - @JsonProperty(value = "user_id", required = true) final long userId, - @JsonProperty(value = "timestamp", required = true) final Date timestamp) { + @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "id", required = true) final UUID id, + @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "user_id", required = true) final long userId, + @JsonProperty(value = "skill_assignment_id", required = true) final String skillAssignmentId, + @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "skill_id", required = true) final String skillId, + @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "subskill_id", required = true) final String subskillId, + @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "question", required = true) final Question question, + @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "question_attempt", required = true) final String questionAttempt, + @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "marks", required = true) final Number marks, + @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "timestamp", required = true) final Date timestamp) { + this.id = id; this.userId = userId; + this.skillAssignmentId = skillAssignmentId; + this.skillId = skillId; + this.subskillId = subskillId; + this.question = question; + this.questionAttempt = questionAttempt; + this.marks = marks; this.timestamp = timestamp; } - /** - * Gets the user ID. - * - * @return the user ID - */ - public long getUserId() { - return userId; - } + public long getUserId() { return userId; } + public String getSkillAssignmentId() { return skillAssignmentId; } + public String getSkillId() { return skillId; } + public String getSubskillId() { return subskillId; } + public String getQuestionAttempt() { return questionAttempt; } + public Number getMarks() { return marks; } + public Date getTimestamp() { return timestamp; } - /** - * Gets the timestamp. - * - * @return the signing timestamp - */ - public Date getTimestamp() { - return timestamp; - } + public record Question( + @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "text", required = true) String text, + @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "answer", required = true) String answer + ) {} } diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java index a6650398ce..258ad67523 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java @@ -182,7 +182,7 @@ static class TestResponse { void assertError(final String message, final Response.Status status) { assertEquals(status.getStatusCode(), response.getStatus()); - assertTrue(this.readEntityAsJsonUnchecked().getString("errorMessage").contains(message)); + assertThat(this.readEntityAsJsonUnchecked().getString("errorMessage")).contains(message); } void assertError(final String message, final String status) { diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index 4bcd2f3e99..dc47ebdafc 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -16,6 +16,7 @@ import jakarta.ws.rs.core.Response; import java.time.Instant; import java.time.temporal.ChronoUnit; +import java.util.UUID; import java.util.function.Consumer; import java.util.stream.Stream; import javax.crypto.Mac; @@ -31,7 +32,9 @@ public class SkillsFacadeIT extends IsaacIntegrationTestWithREST { private static final String HMAC_SECRET = "integration-test-anvil-hmac-secret"; private static final String HMAC_SHA_256 = "HmacSHA256"; - private static final String VALID_PAYLOAD = validPayload(p -> {}); + private static final String VALID_URL = validUrl(); + private static final String VALID_APP_ID = VALID_URL.split("/")[2]; + private static final String VALID_PAYLOAD = validPayload(VALID_APP_ID, p -> {}); private static final String VALID_HMAC = sign(HMAC_SECRET, VALID_PAYLOAD, HMAC_SHA_256); private static final JSONObject VALID_BODY = new JSONObject().put("payload", VALID_PAYLOAD).put("hmac", VALID_HMAC); @@ -70,7 +73,7 @@ class RequestPayloadCheck { @Test public void missingPayload_Returns400() throws Exception { var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var response = client.post(validUrl(), "{}"); + var response = client.post(VALID_URL, "{}"); response.assertError("Invalid JSON object submitted", Response.Status.BAD_REQUEST); } @@ -78,7 +81,7 @@ public void missingPayload_Returns400() throws Exception { public void numericPayload_Returns400() throws Exception { var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); var body = new JSONObject().put("payload", 123).put("hmac", sign(HMAC_SECRET, "123", HMAC_SHA_256)); - var response = client.post(validUrl(), body); + var response = client.post(VALID_URL, body); response.assertError("Invalid payload", Response.Status.BAD_REQUEST); } @@ -86,7 +89,7 @@ public void numericPayload_Returns400() throws Exception { public void extraAttribute_Returns400() throws Exception { var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); var body = new JSONObject().put("payload", VALID_PAYLOAD).put("hmac", VALID_HMAC).put("extra", "value"); - var response = client.post(validUrl(), body); + var response = client.post(VALID_URL, body); response.assertError("Invalid JSON object submitted", Response.Status.BAD_REQUEST); } } @@ -115,24 +118,83 @@ public void invalidBody_Returns400( ) throws Exception { testServer().client() .loginAs(integrationTestUsers.TEST_STUDENT) - .post(validUrl(), body) + .post(VALID_URL, body) .assertError(expectedMessage, Response.Status.BAD_REQUEST); } } @Nested class PayloadContentCheck { + static String MSG_IP = "Invalid payload"; + static Stream invalidPayloads() { return Stream.of( - Arguments.of("missing user_id", validPayload(p -> p.remove("user_id")), "Invalid payload"), - Arguments.of("non-numeric user_id", validPayload(p -> p.put("user_id", "ab")), "Invalid payload"), - Arguments.of("missing timestamp", validPayload(p -> p.remove("timestamp")), "Invalid payload"), - Arguments.of("invalid timestamp", validPayload(p -> p.put("timestamp", "not-a-date")), - "Invalid payload"), - Arguments.of("wrong user_id", validPayload(p -> p.put("user_id", TEST_STUDENT_ID + 1)), + // id + Arguments.of("missing id", validPayload(VALID_APP_ID, p -> p.remove("id")), MSG_IP), + Arguments.of("null id", validPayload(VALID_APP_ID, p -> p.put("id", JSONObject.NULL)), MSG_IP), + Arguments.of("invalid id", validPayload(VALID_APP_ID, p -> p.put("id", "not-uuid")), MSG_IP), + + // user_id + Arguments.of("missing user_id", validPayload(VALID_APP_ID, p -> p.remove("user_id")), MSG_IP), + Arguments.of("null user_id", validPayload(VALID_APP_ID, p -> p.put("user_id", JSONObject.NULL)), + MSG_IP), + Arguments.of("empty user_id", validPayload(VALID_APP_ID, p -> p.put("user_id", "")), MSG_IP), + Arguments.of("non-numeric user_id", validPayload(VALID_APP_ID, p -> p.put("user_id", "ab")), MSG_IP), + Arguments.of("wrong user_id", validPayload(VALID_APP_ID, p -> p.put("user_id", TEST_STUDENT_ID + 1)), "Payload user_id does not match session"), - Arguments.of("expired timestamp", validPayload(p -> p.put("timestamp", "2022-01-01T13:00:00Z")), - "Payload timestamp is outside the allowed window") + + // skill_assignment_id + Arguments.of("missing skill_assignment_id", validPayload(VALID_APP_ID, + p -> p.remove("skill_assignment_id")), MSG_IP), + Arguments.of("non-null skill_assignment_id", validPayload(VALID_APP_ID, + p -> p.put("skill_assignment_id", "some_id")), MSG_IP), + + // skill_id + Arguments.of("missing skill_id", validPayload(VALID_APP_ID, p -> p.remove("skill_id")), MSG_IP), + Arguments.of("null skill_id", validPayload(VALID_APP_ID, p -> p.put("skill_id", JSONObject.NULL)), + MSG_IP), + Arguments.of("wrong skill_id", validPayload(VALID_APP_ID, p -> p.put("skill_id", "wrong_skill_id")), + "Payload skill_id does not match app"), + + // subskill_id + Arguments.of("missing subskill_id", validPayload(VALID_APP_ID, p -> p.remove("subskill_id")), MSG_IP), + Arguments.of("null subskill_id", validPayload(VALID_APP_ID, p -> p.put("subskill_id", JSONObject.NULL)), + MSG_IP), + + // question + Arguments.of("missing question", validPayload(VALID_APP_ID, p -> p.remove("question")), MSG_IP), + Arguments.of("null question", validPayload(VALID_APP_ID, p -> p.put("question", JSONObject.NULL)), + MSG_IP), + Arguments.of("missing question text", validPayload(VALID_APP_ID, + p -> p.getJSONObject("question").remove("text")), MSG_IP), + Arguments.of("null question text", validPayload(VALID_APP_ID, + p -> p.getJSONObject("question").put("text", JSONObject.NULL)), MSG_IP), + Arguments.of("missing question answer", validPayload(VALID_APP_ID, + p -> p.getJSONObject("question").remove("answer")), MSG_IP), + Arguments.of("null question answer", validPayload(VALID_APP_ID, + p -> p.getJSONObject("question").put("answer", JSONObject.NULL)), MSG_IP), + + // question_attempt + Arguments.of("missing question_attempt", validPayload(VALID_APP_ID, + p -> p.remove("question_attempt")), MSG_IP), + Arguments.of("null question_attempt", validPayload(VALID_APP_ID, + p -> p.put("question_attempt", JSONObject.NULL)), MSG_IP), + + // marks + Arguments.of("missing marks", validPayload(VALID_APP_ID, p -> p.remove("marks")), MSG_IP), + Arguments.of("null marks", validPayload(VALID_APP_ID, p -> p.put("marks", JSONObject.NULL)), MSG_IP), + Arguments.of("invalid mark 2", validPayload(VALID_APP_ID, p -> p.put("marks", 2)), + "Payload marks must be 0 or 1"), + Arguments.of("invalid mark -0.4", validPayload(VALID_APP_ID, p -> p.put("marks", -0.4)), + "Payload marks must be 0 or 1"), + + // timestamp + Arguments.of("missing timestamp", validPayload(VALID_APP_ID, p -> p.remove("timestamp")), MSG_IP), + Arguments.of("null timestamp", validPayload(VALID_APP_ID, p -> p.put("timestamp", JSONObject.NULL)), + MSG_IP), + Arguments.of("invalid timestamp", validPayload(VALID_APP_ID, p -> p.put("timestamp", "n/d")), MSG_IP), + Arguments.of("expired timestamp", validPayload(VALID_APP_ID, + p -> p.put("timestamp", "2022-01-01T13:00:00Z")), "Payload timestamp is outside the allowed window") ); } @@ -143,19 +205,19 @@ public void invalidPayload_Returns400( ) throws Exception { testServer().client() .loginAs(integrationTestUsers.TEST_STUDENT) - .post(validUrl(), new JSONObject() + .post(VALID_URL, new JSONObject() .put("payload", payload.toString()) - .put("hmac", sign(HMAC_SECRET, payload.toString(), HMAC_SHA_256)) - ) + .put("hmac", sign(HMAC_SECRET, payload.toString(), HMAC_SHA_256))) .assertError(expectedMessage, Response.Status.BAD_REQUEST); } } @Test public void happy_happy() throws Exception { - var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); - var response = client.post(validUrl(), VALID_BODY); - response.readEntity(String.class); + testServer().client() + .loginAs(integrationTestUsers.TEST_STUDENT) + .post(VALID_URL, VALID_BODY) + .readEntity(String.class); try (var conn = postgresSqlDb.getDatabaseConnection(); var pst = conn.prepareStatement("SELECT COUNT(*) FROM public.skills_question_attempts"); @@ -169,29 +231,40 @@ public void happy_happy() throws Exception { var rs = pst.executeQuery()) { rs.next(); var jsonPayload = new JSONObject(VALID_PAYLOAD); - assertEquals(jsonPayload.getInt("user_id"), rs.getInt("user_id")); + assertEquals(jsonPayload.get("user_id"), rs.getInt("user_id")); assertThat(rs.getTimestamp("timestamp").toInstant()) - .isCloseTo(jsonPayload.getString("timestamp"), within(1, ChronoUnit.MILLIS)); + .isCloseTo(jsonPayload.getString("timestamp"), within(5, ChronoUnit.SECONDS)); } } - private GitContentManager brokenContentManager() throws Exception { + private static GitContentManager brokenContentManager() throws Exception { var brokenClient = ElasticSearchProvider.getClient("localhost", 1, "elastic", "elastic"); var brokenProvider = new ElasticSearchProvider(brokenClient); return new GitContentManager(null, brokenProvider, mainMapper, contentMapper, properties); } - private String validUrl() throws Exception { - var app = elasticHelper.persistJSON(new JSONObject().put("type", "anvilApp")); - return "/skills/" + app.getString("id") + "/answer"; + private static String validUrl() { + try { + var app = elasticHelper.persistJSON(new JSONObject().put("type", "anvilApp")); + return "/skills/" + app.getString("id") + "/answer"; + } catch (final Exception e) { + throw new RuntimeException(e); + } } - private static String validPayload(final Consumer ops) { - var defaultValidPayload = new JSONObject() - .put("user_id", TEST_STUDENT_ID) - .put("timestamp", Instant.now()); - ops.accept(defaultValidPayload); - return defaultValidPayload.toString(); + private static String validPayload(final String skillId, final Consumer op) { + var defaultPayload = new JSONObject() + .put("id", UUID.randomUUID().toString()) + .put("user_id", TEST_STUDENT_ID) + .put("skill_assignment_id", JSONObject.NULL) + .put("skill_id", skillId) + .put("subskill_id", 21) + .put("question", new JSONObject().put("text", "2+2").put("answer", "4")) + .put("question_attempt", "4") + .put("marks", 1) + .put("timestamp", Instant.now().toString()); + op.accept(defaultPayload); + return defaultPayload.toString(); } private static String sign(final String key, final String payload, final String algo) { @@ -199,7 +272,6 @@ private static String sign(final String key, final String payload, final String var signingKey = new SecretKeySpec(key.getBytes(), algo); var mac = Mac.getInstance(algo); mac.init(signingKey); - return new String(Base64.encodeBase64(mac.doFinal(payload.getBytes()))); } catch (final Exception e) { return "NOT_SIGNED"; From 3bdfae848574c224e8bc93928efd29eef1d745cd Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Wed, 17 Jun 2026 12:58:32 +0100 Subject: [PATCH 18/31] finish validating payload content --- .../dtg/isaac/api/managers/SkillsManager.java | 6 - .../cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java | 13 +- .../cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 144 +++++++++--------- 3 files changed, 83 insertions(+), 80 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java index 45491ad352..379b07b22c 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java @@ -74,9 +74,6 @@ public AnvilPayloadDTO parsePayload( ) throws InvalidMarkingResponseException { try { AnvilPayloadDTO dto = objectMapper.readValue(payloadStr, AnvilPayloadDTO.class); - if (dto.getSkillAssignmentId() != null) { - throw new InvalidMarkingResponseException("Invalid payload"); - } if (dto.getUserId() != userId) { throw new InvalidMarkingResponseException("Payload user_id does not match session"); } @@ -86,9 +83,6 @@ public AnvilPayloadDTO parsePayload( if (!dto.getSkillId().equals(appId)) { throw new InvalidMarkingResponseException("Payload skill_id does not match app"); } - if (!(dto.getMarks() instanceof Integer n) || (n != 0 && n != 1)) { - throw new InvalidMarkingResponseException("Payload marks must be 0 or 1"); - } return dto; } catch (final JsonProcessingException e) { throw new InvalidMarkingResponseException("Invalid payload"); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java index d3fa5489e5..25c9143673 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java @@ -4,16 +4,18 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonSetter; import com.fasterxml.jackson.annotation.Nulls; +import org.apache.commons.lang3.Validate; import java.util.Date; import java.util.UUID; +import java.util.stream.Stream; /** * DTO representing the signed payload content from the external Anvil marking server. */ public class AnvilPayloadDTO { private final UUID id; - private final long userId; + private final Long userId; private final String skillAssignmentId; private final String skillId; private final String subskillId; @@ -38,7 +40,7 @@ public class AnvilPayloadDTO { @JsonCreator public AnvilPayloadDTO( @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "id", required = true) final UUID id, - @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "user_id", required = true) final long userId, + @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "user_id", required = true) final Long userId, @JsonProperty(value = "skill_assignment_id", required = true) final String skillAssignmentId, @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "skill_id", required = true) final String skillId, @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "subskill_id", required = true) final String subskillId, @@ -46,6 +48,11 @@ public AnvilPayloadDTO( @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "question_attempt", required = true) final String questionAttempt, @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "marks", required = true) final Number marks, @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "timestamp", required = true) final Date timestamp) { + Validate.isTrue(userId != 0); + Validate.isTrue(skillAssignmentId == null); + Validate.isTrue((marks instanceof Integer n) && (n == 0 || n == 1)); + Stream.of(skillId, subskillId, question.answer, question.text).forEach(Validate::notEmpty); + this.id = id; this.userId = userId; this.skillAssignmentId = skillAssignmentId; @@ -57,7 +64,7 @@ public AnvilPayloadDTO( this.timestamp = timestamp; } - public long getUserId() { return userId; } + public Long getUserId() { return userId; } public String getSkillAssignmentId() { return skillAssignmentId; } public String getSkillId() { return skillId; } public String getSubskillId() { return subskillId; } diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index dc47ebdafc..e984d93c78 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -34,7 +34,7 @@ public class SkillsFacadeIT extends IsaacIntegrationTestWithREST { private static final String HMAC_SHA_256 = "HmacSHA256"; private static final String VALID_URL = validUrl(); private static final String VALID_APP_ID = VALID_URL.split("/")[2]; - private static final String VALID_PAYLOAD = validPayload(VALID_APP_ID, p -> {}); + private static final String VALID_PAYLOAD = validPayload(p -> {}); private static final String VALID_HMAC = sign(HMAC_SECRET, VALID_PAYLOAD, HMAC_SHA_256); private static final JSONObject VALID_BODY = new JSONObject().put("payload", VALID_PAYLOAD).put("hmac", VALID_HMAC); @@ -128,74 +128,70 @@ class PayloadContentCheck { static String MSG_IP = "Invalid payload"; static Stream invalidPayloads() { - return Stream.of( - // id - Arguments.of("missing id", validPayload(VALID_APP_ID, p -> p.remove("id")), MSG_IP), - Arguments.of("null id", validPayload(VALID_APP_ID, p -> p.put("id", JSONObject.NULL)), MSG_IP), - Arguments.of("invalid id", validPayload(VALID_APP_ID, p -> p.put("id", "not-uuid")), MSG_IP), - - // user_id - Arguments.of("missing user_id", validPayload(VALID_APP_ID, p -> p.remove("user_id")), MSG_IP), - Arguments.of("null user_id", validPayload(VALID_APP_ID, p -> p.put("user_id", JSONObject.NULL)), - MSG_IP), - Arguments.of("empty user_id", validPayload(VALID_APP_ID, p -> p.put("user_id", "")), MSG_IP), - Arguments.of("non-numeric user_id", validPayload(VALID_APP_ID, p -> p.put("user_id", "ab")), MSG_IP), - Arguments.of("wrong user_id", validPayload(VALID_APP_ID, p -> p.put("user_id", TEST_STUDENT_ID + 1)), - "Payload user_id does not match session"), - - // skill_assignment_id - Arguments.of("missing skill_assignment_id", validPayload(VALID_APP_ID, - p -> p.remove("skill_assignment_id")), MSG_IP), - Arguments.of("non-null skill_assignment_id", validPayload(VALID_APP_ID, - p -> p.put("skill_assignment_id", "some_id")), MSG_IP), - - // skill_id - Arguments.of("missing skill_id", validPayload(VALID_APP_ID, p -> p.remove("skill_id")), MSG_IP), - Arguments.of("null skill_id", validPayload(VALID_APP_ID, p -> p.put("skill_id", JSONObject.NULL)), - MSG_IP), - Arguments.of("wrong skill_id", validPayload(VALID_APP_ID, p -> p.put("skill_id", "wrong_skill_id")), - "Payload skill_id does not match app"), - - // subskill_id - Arguments.of("missing subskill_id", validPayload(VALID_APP_ID, p -> p.remove("subskill_id")), MSG_IP), - Arguments.of("null subskill_id", validPayload(VALID_APP_ID, p -> p.put("subskill_id", JSONObject.NULL)), - MSG_IP), - - // question - Arguments.of("missing question", validPayload(VALID_APP_ID, p -> p.remove("question")), MSG_IP), - Arguments.of("null question", validPayload(VALID_APP_ID, p -> p.put("question", JSONObject.NULL)), - MSG_IP), - Arguments.of("missing question text", validPayload(VALID_APP_ID, - p -> p.getJSONObject("question").remove("text")), MSG_IP), - Arguments.of("null question text", validPayload(VALID_APP_ID, - p -> p.getJSONObject("question").put("text", JSONObject.NULL)), MSG_IP), - Arguments.of("missing question answer", validPayload(VALID_APP_ID, - p -> p.getJSONObject("question").remove("answer")), MSG_IP), - Arguments.of("null question answer", validPayload(VALID_APP_ID, - p -> p.getJSONObject("question").put("answer", JSONObject.NULL)), MSG_IP), - - // question_attempt - Arguments.of("missing question_attempt", validPayload(VALID_APP_ID, - p -> p.remove("question_attempt")), MSG_IP), - Arguments.of("null question_attempt", validPayload(VALID_APP_ID, - p -> p.put("question_attempt", JSONObject.NULL)), MSG_IP), - - // marks - Arguments.of("missing marks", validPayload(VALID_APP_ID, p -> p.remove("marks")), MSG_IP), - Arguments.of("null marks", validPayload(VALID_APP_ID, p -> p.put("marks", JSONObject.NULL)), MSG_IP), - Arguments.of("invalid mark 2", validPayload(VALID_APP_ID, p -> p.put("marks", 2)), - "Payload marks must be 0 or 1"), - Arguments.of("invalid mark -0.4", validPayload(VALID_APP_ID, p -> p.put("marks", -0.4)), - "Payload marks must be 0 or 1"), - - // timestamp - Arguments.of("missing timestamp", validPayload(VALID_APP_ID, p -> p.remove("timestamp")), MSG_IP), - Arguments.of("null timestamp", validPayload(VALID_APP_ID, p -> p.put("timestamp", JSONObject.NULL)), - MSG_IP), - Arguments.of("invalid timestamp", validPayload(VALID_APP_ID, p -> p.put("timestamp", "n/d")), MSG_IP), - Arguments.of("expired timestamp", validPayload(VALID_APP_ID, - p -> p.put("timestamp", "2022-01-01T13:00:00Z")), "Payload timestamp is outside the allowed window") - ); + var s = Stream.builder(); + + // id + addNonEmptyCasesFor("id", s); + s.add(Arguments.of("invalid id", validPayload(p -> p.put("id", "not-uuid")), MSG_IP)); + + // user_id + addNonEmptyCasesFor("user_id", s); + s.add(Arguments.of("non-numeric user_id", validPayload(p -> p.put("user_id", "ab")), MSG_IP)); + s.add(Arguments.of("wrong user_id", validPayload(p -> p.put("user_id", TEST_STUDENT_ID + 1)), + "Payload user_id does not match session")); + + // skill_assignment_id + s.add(Arguments.of("missing skill_assignment_id", validPayload(p -> p.remove("skill_assignment_id")), + MSG_IP)); + s.add(Arguments.of("non-null skill_assignment_id", + validPayload(p -> p.put("skill_assignment_id", "some_id")), MSG_IP)); + + // skill_id + addNonEmptyCasesFor("skill_id", s); + s.add(Arguments.of("wrong skill_id", validPayload(p -> p.put("skill_id", "wrong_skill_id")), + "Payload skill_id does not match app")); + + // subskill_id + addNonEmptyCasesFor("subskill_id", s); + + // question + addNonEmptyCasesFor("question", s); + s.add(Arguments.of("missing question text", validPayload(p -> p.getJSONObject("question").remove("text")), + MSG_IP)); + s.add(Arguments.of("null question text", + validPayload(p -> p.getJSONObject("question").put("text", JSONObject.NULL)), MSG_IP)); + s.add(Arguments.of("empty question text", + validPayload(p -> p.getJSONObject("question").put("text", "")), MSG_IP)); + s.add(Arguments.of("missing question text", + validPayload(p -> p.getJSONObject("question").remove("answer")), MSG_IP)); + s.add(Arguments.of("null question answer", + validPayload(p -> p.getJSONObject("question").put("answer", JSONObject.NULL)), MSG_IP)); + s.add(Arguments.of("empty question answer", + validPayload(p -> p.getJSONObject("question").put("answer", "")), MSG_IP)); + s.add(Arguments.of("extra value on question", + validPayload(p -> p.getJSONObject("question").put("extra", "value")), MSG_IP)); + + // question_attempt + s.add(Arguments.of("missing question_attempt", validPayload( + p -> p.remove("question_attempt")), MSG_IP)); + s.add(Arguments.of("null question_attempt", validPayload( + p -> p.put("question_attempt", JSONObject.NULL)), MSG_IP)); + + // marks + addNonEmptyCasesFor("marks", s); + s.add(Arguments.of("invalid mark 2", validPayload(p -> p.put("marks", 2)), MSG_IP)); + s.add(Arguments.of("invalid mark -0.4", validPayload(p -> p.put("marks", -0.4)), MSG_IP)); + + // timestamp + addNonEmptyCasesFor("timestamp", s); + s.add(Arguments.of("invalid timestamp", validPayload(p -> p.put("timestamp", "n/d")), MSG_IP)); + s.add(Arguments.of("expired timestamp", validPayload(p -> p.put("timestamp", "2022-01-01T13:00:00Z")), + "Payload timestamp is outside the allowed window")); + + // other cases + s.add(Arguments.of("extra value", validPayload(p -> p.put("extra", "value")), MSG_IP)); + + return s.build(); } @ParameterizedTest(name = "{0}") @@ -210,6 +206,12 @@ public void invalidPayload_Returns400( .put("hmac", sign(HMAC_SECRET, payload.toString(), HMAC_SHA_256))) .assertError(expectedMessage, Response.Status.BAD_REQUEST); } + + private static void addNonEmptyCasesFor(final String fieldName, final Stream.Builder s) { + s.add(Arguments.of("missing " + fieldName, validPayload(p -> p.remove(fieldName)), MSG_IP)); + s.add(Arguments.of("null " + fieldName, validPayload(p -> p.put(fieldName, JSONObject.NULL)), MSG_IP)); + s.add(Arguments.of("empty " + fieldName, validPayload(p -> p.put(fieldName, "")), MSG_IP)); + } } @Test @@ -252,12 +254,12 @@ private static String validUrl() { } } - private static String validPayload(final String skillId, final Consumer op) { + private static String validPayload(final Consumer op) { var defaultPayload = new JSONObject() .put("id", UUID.randomUUID().toString()) .put("user_id", TEST_STUDENT_ID) .put("skill_assignment_id", JSONObject.NULL) - .put("skill_id", skillId) + .put("skill_id", SkillsFacadeIT.VALID_APP_ID) .put("subskill_id", 21) .put("question", new JSONObject().put("text", "2+2").put("answer", "4")) .put("question_attempt", "4") From eb8d3e21cd9c9db41639fa0be744defffcdc2e0a Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Wed, 17 Jun 2026 14:46:20 +0100 Subject: [PATCH 19/31] disallow large payloads --- .../ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java | 4 ++++ .../java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java index 379b07b22c..e7eaf65c95 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java @@ -73,6 +73,10 @@ public AnvilPayloadDTO parsePayload( final String payloadStr, final long userId, final String appId ) throws InvalidMarkingResponseException { try { + if (payloadStr.length() > 10 * 1024) { + throw new InvalidMarkingResponseException("Payload too large"); + } + AnvilPayloadDTO dto = objectMapper.readValue(payloadStr, AnvilPayloadDTO.class); if (dto.getUserId() != userId) { throw new InvalidMarkingResponseException("Payload user_id does not match session"); diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index e984d93c78..b69370c861 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -92,6 +92,14 @@ public void extraAttribute_Returns400() throws Exception { var response = client.post(VALID_URL, body); response.assertError("Invalid JSON object submitted", Response.Status.BAD_REQUEST); } + + @Test + public void oversizedPayload_Returns400() throws Exception { + var large = validPayload(p -> p.put("question_attempt", "x".repeat(10 * 1024 + 1))); + var body = new JSONObject().put("payload", large).put("hmac", sign(HMAC_SECRET, large, HMAC_SHA_256)); + var response = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT).post(VALID_URL, body); + response.assertError("Payload too large", Response.Status.BAD_REQUEST); + } } @Nested From d9f141ef5eb161832e8a249f3bcb8becca12950b Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Wed, 17 Jun 2026 15:11:44 +0100 Subject: [PATCH 20/31] use framework to parse request body --- .../uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java | 3 +-- .../cl/dtg/isaac/api/managers/SkillsManager.java | 15 --------------- .../isaac/api/IsaacIntegrationTestWithREST.java | 9 +++++++++ .../ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 8 ++++---- 4 files changed, 14 insertions(+), 21 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java index 6c1d126dee..7852200640 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java @@ -65,7 +65,7 @@ public SkillsFacade(final AbstractConfigLoader properties, final UserAccountMana @Consumes(MediaType.APPLICATION_JSON) public Response answerQuestion(@Context final HttpServletRequest request, @PathParam("appId") final String appId, - final String body) { + final AnvilMarkingResponseDTO markingResponse) { try { RegisteredUserDTO currentUser = userManager.getCurrentRegisteredUser(request); if (!(this.contentManager.getContentDOById(appId) instanceof AnvilApp)) { @@ -74,7 +74,6 @@ public Response answerQuestion(@Context final HttpServletRequest request, return error.toResponse(); } - AnvilMarkingResponseDTO markingResponse = skillsManager.parseRequest(body); if (!skillsManager.isHmacValid(markingResponse)) { return new SegueErrorResponse(Status.BAD_REQUEST, "Invalid HMAC signature").toResponse(); } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java index e7eaf65c95..631ed924e2 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java @@ -35,21 +35,6 @@ public SkillsManager(final AbstractConfigLoader properties, final ISkillsAttempt this.skillsAttemptManager = skillsAttemptManager; } - /** - * Parses and validates the raw JSON body from the external marking server. - * - * @param body - the raw JSON request body - * @return the deserialised marking response - * @throws InvalidMarkingResponseException if the body is missing or malformed - */ - public AnvilMarkingResponseDTO parseRequest(final String body) throws InvalidMarkingResponseException { - try { - return objectMapper.readValue(body, AnvilMarkingResponseDTO.class); - } catch (final JsonProcessingException e) { - throw new InvalidMarkingResponseException("Invalid JSON object submitted"); - } - } - /** * Verifies that the HMAC in the DTO matches the expected signature of the payload. * diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java index 258ad67523..162ef85418 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java @@ -131,6 +131,15 @@ public static class TestApp extends Application { public Set getSingletons() { return TestApp.facades; } + + @Override + public Set> getClasses() { + return Set.of( + uk.ac.cam.cl.dtg.isaac.configuration.RestEasyJacksonConfiguration.class, + uk.ac.cam.cl.dtg.segue.configuration.exceptionMappers.JacksonExceptionMapper.class, + uk.ac.cam.cl.dtg.isaac.configuration.exceptionMappers.UnhandledExceptionMapper.class + ); + } } } diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index b69370c861..03d90ee25a 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -40,7 +40,7 @@ public class SkillsFacadeIT extends IsaacIntegrationTestWithREST { @Test public void notLoggedIn_Returns401() throws Exception { - var response = testServer().client().post("/skills/unknown_app/answer", "{}"); + var response = testServer().client().post("/skills/unknown_app/answer", VALID_BODY); response.assertError("You must be logged in to access this resource.", Response.Status.UNAUTHORIZED); } @@ -74,7 +74,7 @@ class RequestPayloadCheck { public void missingPayload_Returns400() throws Exception { var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); var response = client.post(VALID_URL, "{}"); - response.assertError("Invalid JSON object submitted", Response.Status.BAD_REQUEST); + response.assertError("Invalid JSON provided!", Response.Status.BAD_REQUEST); } @Test @@ -90,7 +90,7 @@ public void extraAttribute_Returns400() throws Exception { var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); var body = new JSONObject().put("payload", VALID_PAYLOAD).put("hmac", VALID_HMAC).put("extra", "value"); var response = client.post(VALID_URL, body); - response.assertError("Invalid JSON object submitted", Response.Status.BAD_REQUEST); + response.assertError("Invalid JSON provided!", Response.Status.BAD_REQUEST); } @Test @@ -107,7 +107,7 @@ class HmacVerification { static Stream invalidBodies() { return Stream.of( Arguments.of("missing hmac", new JSONObject().put("payload", VALID_PAYLOAD), - "Invalid JSON object submitted"), + "Invalid JSON provided!"), Arguments.of("invalid hmac", new JSONObject().put("payload", VALID_PAYLOAD) .put("hmac", "not-a-valid-signature"), "Invalid HMAC signature"), Arguments.of("wrong secret", new JSONObject().put("payload", VALID_PAYLOAD) From dd5024510ac0f057bb334a71aecf0890da53a20a Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Wed, 17 Jun 2026 15:40:09 +0100 Subject: [PATCH 21/31] attach skills facade --- .../IsaacApplicationRegister.java | 2 ++ .../SegueGuiceConfigurationModule.java | 22 +++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/configuration/IsaacApplicationRegister.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/configuration/IsaacApplicationRegister.java index a540be9203..9d9bce7ffd 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/configuration/IsaacApplicationRegister.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/configuration/IsaacApplicationRegister.java @@ -34,6 +34,7 @@ import uk.ac.cam.cl.dtg.isaac.api.IsaacController; import uk.ac.cam.cl.dtg.isaac.api.PagesFacade; import uk.ac.cam.cl.dtg.isaac.api.QuizFacade; +import uk.ac.cam.cl.dtg.isaac.api.SkillsFacade; import uk.ac.cam.cl.dtg.segue.api.AdminFacade; import uk.ac.cam.cl.dtg.segue.api.AuthenticationFacade; import uk.ac.cam.cl.dtg.segue.api.AuthorisationFacade; @@ -123,6 +124,7 @@ public final Set getSingletons() { this.singletons.add(injector.getInstance(EmailFacade.class)); this.singletons.add(injector.getInstance(QuizFacade.class)); this.singletons.add(injector.getInstance(BookmarksFacade.class)); + this.singletons.add(injector.getInstance(SkillsFacade.class)); // initialise filters this.singletons.add(injector.getInstance(PerformanceMonitor.class)); diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/configuration/SegueGuiceConfigurationModule.java b/src/main/java/uk/ac/cam/cl/dtg/segue/configuration/SegueGuiceConfigurationModule.java index fabccbeda2..17d11e680b 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/configuration/SegueGuiceConfigurationModule.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/configuration/SegueGuiceConfigurationModule.java @@ -38,6 +38,7 @@ import org.reflections.Reflections; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import uk.ac.cam.cl.dtg.isaac.ISkillsAttemptManager; import uk.ac.cam.cl.dtg.isaac.api.managers.AssignmentManager; import uk.ac.cam.cl.dtg.isaac.api.managers.GameManager; import uk.ac.cam.cl.dtg.isaac.api.managers.QuizAssignmentManager; @@ -55,6 +56,7 @@ import uk.ac.cam.cl.dtg.isaac.dao.PgQuizAssignmentPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dao.PgQuizAttemptPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dao.PgQuizQuestionAttemptPersistenceManager; +import uk.ac.cam.cl.dtg.isaac.dao.PgSkillsAttemptManager; import uk.ac.cam.cl.dtg.isaac.dos.AbstractUserPreferenceManager; import uk.ac.cam.cl.dtg.isaac.dos.ILocationHistory; import uk.ac.cam.cl.dtg.isaac.dos.IUserAlerts; @@ -191,6 +193,7 @@ public class SegueGuiceConfigurationModule extends AbstractModule implements Ser private static UserAccountManager userManager = null; private static UserAuthenticationManager userAuthenticationManager = null; private static IQuestionAttemptManager questionPersistenceManager = null; + private static ISkillsAttemptManager skillsAttemptManager = null; private static SegueJobService segueJobService = null; private static ILogManager logManager; @@ -874,6 +877,25 @@ private IQuestionAttemptManager getQuestionManager(final PostgresSqlDb ds, final return questionPersistenceManager; } + /** + * SkillAttemptManager. + * + * @param ds - postgres data source + * @return a singleton for question persistence. + */ + @Inject + @Provides + @Singleton + private ISkillsAttemptManager getSkillAttemptManager(final PostgresSqlDb ds) { + // this needs to be a singleton as it provides a temporary cache for anonymous question attempts. + if (null == skillsAttemptManager) { + skillsAttemptManager = new PgSkillsAttemptManager(ds); + log.info("Creating singleton of ISkillsAttemptManager"); + } + + return skillsAttemptManager; + } + /** * This provides a singleton of the GroupManager. * From 7e66d85c2adbd57954b234d5ce1f85bb3241a8c6 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Wed, 17 Jun 2026 16:27:13 +0100 Subject: [PATCH 22/31] just require anvil child, root is always page --- .../java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java | 9 ++++++++- .../java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 8 ++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java index 7852200640..8a0e4e3530 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java @@ -7,6 +7,7 @@ import uk.ac.cam.cl.dtg.isaac.api.managers.InvalidMarkingResponseException; import uk.ac.cam.cl.dtg.isaac.api.managers.SkillsManager; import uk.ac.cam.cl.dtg.isaac.dos.content.AnvilApp; +import uk.ac.cam.cl.dtg.isaac.dos.content.Content; import uk.ac.cam.cl.dtg.isaac.dto.AnvilMarkingResponseDTO; import uk.ac.cam.cl.dtg.isaac.dto.SegueErrorResponse; import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; @@ -68,7 +69,7 @@ public Response answerQuestion(@Context final HttpServletRequest request, final AnvilMarkingResponseDTO markingResponse) { try { RegisteredUserDTO currentUser = userManager.getCurrentRegisteredUser(request); - if (!(this.contentManager.getContentDOById(appId) instanceof AnvilApp)) { + if (!(hasAnvilApp(this.contentManager.getContentDOById(appId)))) { var error = new SegueErrorResponse(Status.NOT_FOUND, "No app found for given id: " + appId); log.warn(error.getErrorMessage()); return error.toResponse(); @@ -92,4 +93,10 @@ public Response answerQuestion(@Context final HttpServletRequest request, return error.toResponse(); } } + + private boolean hasAnvilApp(final Content content) { + return content instanceof AnvilApp + || (content != null + && content.getChildren().stream().anyMatch(c -> c instanceof Content cc && hasAnvilApp(cc))); + } } diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index 03d90ee25a..ca6844376f 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -1,6 +1,7 @@ package uk.ac.cam.cl.dtg.isaac.api; import org.apache.commons.codec.binary.Base64; +import org.json.JSONArray; import org.json.JSONObject; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -255,8 +256,11 @@ private static GitContentManager brokenContentManager() throws Exception { private static String validUrl() { try { - var app = elasticHelper.persistJSON(new JSONObject().put("type", "anvilApp")); - return "/skills/" + app.getString("id") + "/answer"; + var page = elasticHelper.persistJSON(new JSONObject() + .put("type", "page") + .put("children", new JSONArray().put(new JSONObject().put("type", "anvilApp"))) + ); + return "/skills/" + page.getString("id") + "/answer"; } catch (final Exception e) { throw new RuntimeException(e); } From 642d247ecc6b36c934e4f79ff5129bbd1f413551 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Wed, 17 Jun 2026 17:17:36 +0100 Subject: [PATCH 23/31] record answers to database --- .../dtg/isaac/dao/PgSkillsAttemptManager.java | 19 +++++++++++++++---- .../cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java | 10 ++++++++++ .../2026-06-skills_question_attempts.sql | 8 ++++++++ .../postgres-rutherford-create-script.sql | 12 ++++++++++-- .../api/IsaacIntegrationTestWithREST.java | 1 - .../cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 17 +++++++++++++---- 6 files changed, 56 insertions(+), 11 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java index 0a1471f051..d07a57931b 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java @@ -19,10 +19,21 @@ public PgSkillsAttemptManager(final PostgresSqlDb database) { @Override public void registerSkillsAttempt(final AnvilPayloadDTO attempt) { try (var conn = database.getDatabaseConnection(); - var pst = conn.prepareStatement( - "INSERT INTO skills_question_attempts (user_id, timestamp) VALUES (?, ?)")) { - pst.setLong(1, attempt.getUserId()); - pst.setTimestamp(2, new Timestamp(attempt.getTimestamp().getTime())); + var pst = conn.prepareStatement(""" + INSERT INTO skills_question_attempts ( + id, user_id, skill_assignment_id, skill_id, subskill_id, question_text, question_answer, + question_attempt, marks, timestamp + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)""")) { + pst.setString(1, attempt.getId().toString()); + pst.setLong(2, attempt.getUserId()); + pst.setString(3, attempt.getSkillAssignmentId()); + pst.setString(4, attempt.getSkillId()); + pst.setString(5, attempt.getSubskillId()); + pst.setString(6, attempt.getQuestion().text()); + pst.setString(7, attempt.getQuestion().answer()); + pst.setString(8, attempt.getQuestionAttempt()); + pst.setInt(9, (Integer) attempt.getMarks()); + pst.setTimestamp(10, new Timestamp(attempt.getTimestamp().getTime())); pst.executeUpdate(); } catch (final SQLException e) { throw new RuntimeException("Failed to record skills attempt", e); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java index 25c9143673..a3ea4bf661 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java @@ -64,12 +64,22 @@ public AnvilPayloadDTO( this.timestamp = timestamp; } + public UUID getId() { return id; } + public Long getUserId() { return userId; } + public String getSkillAssignmentId() { return skillAssignmentId; } + public String getSkillId() { return skillId; } + public String getSubskillId() { return subskillId; } + + public Question getQuestion() { return question; } + public String getQuestionAttempt() { return questionAttempt; } + public Number getMarks() { return marks; } + public Date getTimestamp() { return timestamp; } public record Question( diff --git a/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql b/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql index 39b71ed88e..51f2e5984f 100644 --- a/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql +++ b/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql @@ -1,5 +1,13 @@ CREATE TABLE public.skills_question_attempts ( + id TEXT NOT NULL, user_id INTEGER NOT NULL, + skill_assignment_id TEXT, + skill_id TEXT NOT NULL, + subskill_id TEXT NOT NULL, + question_text TEXT NOT NULL, + question_answer TEXT NOT NULL, + question_attempt TEXT NOT NULL, + marks INTEGER NOT NULL, timestamp TIMESTAMP WITH TIME ZONE NOT NULL ); diff --git a/src/main/resources/db_scripts/postgres-rutherford-create-script.sql b/src/main/resources/db_scripts/postgres-rutherford-create-script.sql index 6f92e2c68a..e5e44adc97 100644 --- a/src/main/resources/db_scripts/postgres-rutherford-create-script.sql +++ b/src/main/resources/db_scripts/postgres-rutherford-create-script.sql @@ -603,8 +603,16 @@ ALTER TABLE public.user_bookmarks OWNER TO rutherford; -- Name: skill_question_attempts; Type: TABLE; Schema: public; Owner: rutherford -- CREATE TABLE public.skills_question_attempts ( - user_id INTEGER NOT NULL, - timestamp TIMESTAMP WITH TIME ZONE NOT NULL + id TEXT NOT NULL, + user_id INTEGER NOT NULL, + skill_assignment_id TEXT, + skill_id TEXT NOT NULL, + subskill_id TEXT NOT NULL, + question_text TEXT NOT NULL, + question_answer TEXT NOT NULL, + question_attempt TEXT NOT NULL, + marks INTEGER NOT NULL, + timestamp TIMESTAMP WITH TIME ZONE NOT NULL ); ALTER TABLE public.skills_question_attempts OWNER to rutherford; diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java index 162ef85418..df3d323b3a 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java @@ -31,7 +31,6 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertTrue; import static uk.ac.cam.cl.dtg.segue.api.Constants.*; /** diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index ca6844376f..8c5b794299 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -26,6 +26,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.within; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; import static uk.ac.cam.cl.dtg.isaac.api.ITConstants.REGRESSION_TEST_PAGE_ID; import static uk.ac.cam.cl.dtg.isaac.api.ITConstants.TEST_STUDENT_ID; @@ -238,13 +239,21 @@ public void happy_happy() throws Exception { } try (var conn = postgresSqlDb.getDatabaseConnection(); - var pst = conn.prepareStatement("SELECT user_id, timestamp FROM public.skills_question_attempts"); + var pst = conn.prepareStatement("SELECT * FROM public.skills_question_attempts"); var rs = pst.executeQuery()) { rs.next(); - var jsonPayload = new JSONObject(VALID_PAYLOAD); - assertEquals(jsonPayload.get("user_id"), rs.getInt("user_id")); + var p = new JSONObject(VALID_PAYLOAD); + assertEquals(p.getString("id"), rs.getString("id")); + assertEquals(p.getInt("user_id"), rs.getInt("user_id")); + assertNull(rs.getString("skill_assignment_id")); + assertEquals(p.getString("skill_id"), rs.getString("skill_id")); + assertEquals(p.get("subskill_id").toString(), rs.getString("subskill_id")); + assertEquals(p.getJSONObject("question").getString("text"), rs.getString("question_text")); + assertEquals(p.getJSONObject("question").getString("answer"), rs.getString("question_answer")); + assertEquals(p.getString("question_attempt"), rs.getString("question_attempt")); + assertEquals(p.getInt("marks"), rs.getInt("marks")); assertThat(rs.getTimestamp("timestamp").toInstant()) - .isCloseTo(jsonPayload.getString("timestamp"), within(5, ChronoUnit.SECONDS)); + .isCloseTo(p.getString("timestamp"), within(5, ChronoUnit.SECONDS)); } } From 273a7a915fdcc028252021e230305f9cdcc6284b Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Thu, 18 Jun 2026 13:30:25 +0100 Subject: [PATCH 24/31] treat 2 question fields as opaque json --- .../dtg/isaac/dao/PgSkillsAttemptManager.java | 15 +++-- .../cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java | 59 ++++++++++++------- .../2026-06-skills_question_attempts.sql | 5 +- .../postgres-rutherford-create-script.sql | 5 +- .../cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 31 ++++------ 5 files changed, 59 insertions(+), 56 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java index d07a57931b..88e5b2cf79 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java @@ -21,19 +21,18 @@ public void registerSkillsAttempt(final AnvilPayloadDTO attempt) { try (var conn = database.getDatabaseConnection(); var pst = conn.prepareStatement(""" INSERT INTO skills_question_attempts ( - id, user_id, skill_assignment_id, skill_id, subskill_id, question_text, question_answer, - question_attempt, marks, timestamp - ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)""")) { + id, user_id, skill_assignment_id, skill_id, subskill_id, question, question_attempt, marks, + timestamp + ) VALUES (?, ?, ?, ?, ?, (?::jsonb), (?::jsonb), ?, ?)""")) { pst.setString(1, attempt.getId().toString()); pst.setLong(2, attempt.getUserId()); pst.setString(3, attempt.getSkillAssignmentId()); pst.setString(4, attempt.getSkillId()); pst.setString(5, attempt.getSubskillId()); - pst.setString(6, attempt.getQuestion().text()); - pst.setString(7, attempt.getQuestion().answer()); - pst.setString(8, attempt.getQuestionAttempt()); - pst.setInt(9, (Integer) attempt.getMarks()); - pst.setTimestamp(10, new Timestamp(attempt.getTimestamp().getTime())); + pst.setString(6, attempt.getQuestion()); + pst.setString(7, attempt.getQuestionAttempt()); + pst.setInt(8, (Integer) attempt.getMarks()); + pst.setTimestamp(9, new Timestamp(attempt.getTimestamp().getTime())); pst.executeUpdate(); } catch (final SQLException e) { throw new RuntimeException("Failed to record skills attempt", e); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java index a3ea4bf661..e46103e1c5 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java @@ -4,6 +4,7 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonSetter; import com.fasterxml.jackson.annotation.Nulls; +import com.fasterxml.jackson.databind.JsonNode; import org.apache.commons.lang3.Validate; import java.util.Date; @@ -19,8 +20,8 @@ public class AnvilPayloadDTO { private final String skillAssignmentId; private final String skillId; private final String subskillId; - private final Question question; - private final String questionAttempt; + private final JsonNode question; + private final JsonNode questionAttempt; private final Number marks; private final Date timestamp; @@ -43,19 +44,22 @@ public AnvilPayloadDTO( @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "user_id", required = true) final Long userId, @JsonProperty(value = "skill_assignment_id", required = true) final String skillAssignmentId, @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "skill_id", required = true) final String skillId, - @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "subskill_id", required = true) final String subskillId, - @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "question", required = true) final Question question, - @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "question_attempt", required = true) final String questionAttempt, + @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "subskill_id", required = true) + final String subskillId, + @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "question", required = true) final JsonNode question, + @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "question_attempt", required = true) + final JsonNode questionAttempt, @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "marks", required = true) final Number marks, @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "timestamp", required = true) final Date timestamp) { Validate.isTrue(userId != 0); Validate.isTrue(skillAssignmentId == null); Validate.isTrue((marks instanceof Integer n) && (n == 0 || n == 1)); - Stream.of(skillId, subskillId, question.answer, question.text).forEach(Validate::notEmpty); + Stream.of(question, questionAttempt).forEach(f -> Validate.isTrue(f.isObject())); + Stream.of(skillId, subskillId).forEach(Validate::notEmpty); this.id = id; this.userId = userId; - this.skillAssignmentId = skillAssignmentId; + this.skillAssignmentId = null; this.skillId = skillId; this.subskillId = subskillId; this.question = question; @@ -64,26 +68,39 @@ public AnvilPayloadDTO( this.timestamp = timestamp; } - public UUID getId() { return id; } - - public Long getUserId() { return userId; } + public UUID getId() { + return id; + } - public String getSkillAssignmentId() { return skillAssignmentId; } + public Long getUserId() { + return userId; + } - public String getSkillId() { return skillId; } + public String getSkillAssignmentId() { + return skillAssignmentId; + } - public String getSubskillId() { return subskillId; } + public String getSkillId() { + return skillId; + } - public Question getQuestion() { return question; } + public String getSubskillId() { + return subskillId; + } - public String getQuestionAttempt() { return questionAttempt; } + public String getQuestion() { + return question.toString(); + } - public Number getMarks() { return marks; } + public String getQuestionAttempt() { + return questionAttempt.toString(); + } - public Date getTimestamp() { return timestamp; } + public Number getMarks() { + return marks; + } - public record Question( - @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "text", required = true) String text, - @JsonSetter(nulls = Nulls.FAIL) @JsonProperty(value = "answer", required = true) String answer - ) {} + public Date getTimestamp() { + return timestamp; + } } diff --git a/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql b/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql index 51f2e5984f..5e6022a39e 100644 --- a/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql +++ b/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql @@ -4,9 +4,8 @@ CREATE TABLE public.skills_question_attempts ( skill_assignment_id TEXT, skill_id TEXT NOT NULL, subskill_id TEXT NOT NULL, - question_text TEXT NOT NULL, - question_answer TEXT NOT NULL, - question_attempt TEXT NOT NULL, + question JSONB NOT NULL, + question_attempt JSONB NOT NULL, marks INTEGER NOT NULL, timestamp TIMESTAMP WITH TIME ZONE NOT NULL ); diff --git a/src/main/resources/db_scripts/postgres-rutherford-create-script.sql b/src/main/resources/db_scripts/postgres-rutherford-create-script.sql index e5e44adc97..5daba22844 100644 --- a/src/main/resources/db_scripts/postgres-rutherford-create-script.sql +++ b/src/main/resources/db_scripts/postgres-rutherford-create-script.sql @@ -608,9 +608,8 @@ CREATE TABLE public.skills_question_attempts ( skill_assignment_id TEXT, skill_id TEXT NOT NULL, subskill_id TEXT NOT NULL, - question_text TEXT NOT NULL, - question_answer TEXT NOT NULL, - question_attempt TEXT NOT NULL, + question JSONB NOT NULL, + question_attempt JSONB NOT NULL, marks INTEGER NOT NULL, timestamp TIMESTAMP WITH TIME ZONE NOT NULL ); diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index 8c5b794299..bec0579877 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -166,26 +166,12 @@ static Stream invalidPayloads() { // question addNonEmptyCasesFor("question", s); - s.add(Arguments.of("missing question text", validPayload(p -> p.getJSONObject("question").remove("text")), - MSG_IP)); - s.add(Arguments.of("null question text", - validPayload(p -> p.getJSONObject("question").put("text", JSONObject.NULL)), MSG_IP)); - s.add(Arguments.of("empty question text", - validPayload(p -> p.getJSONObject("question").put("text", "")), MSG_IP)); - s.add(Arguments.of("missing question text", - validPayload(p -> p.getJSONObject("question").remove("answer")), MSG_IP)); - s.add(Arguments.of("null question answer", - validPayload(p -> p.getJSONObject("question").put("answer", JSONObject.NULL)), MSG_IP)); - s.add(Arguments.of("empty question answer", - validPayload(p -> p.getJSONObject("question").put("answer", "")), MSG_IP)); - s.add(Arguments.of("extra value on question", - validPayload(p -> p.getJSONObject("question").put("extra", "value")), MSG_IP)); + s.add(Arguments.of("invalid question JSON, str", validPayload(p -> p.put("question", "ab")), MSG_IP)); + s.add(Arguments.of("invalid question JSON, number", validPayload(p -> p.put("question", 1)), MSG_IP)); // question_attempt - s.add(Arguments.of("missing question_attempt", validPayload( - p -> p.remove("question_attempt")), MSG_IP)); - s.add(Arguments.of("null question_attempt", validPayload( - p -> p.put("question_attempt", JSONObject.NULL)), MSG_IP)); + addNonEmptyCasesFor("question_attempt", s); + s.add(Arguments.of("invalid attempts JSON", validPayload(p -> p.put("question_attempt", "a")), MSG_IP)); // marks addNonEmptyCasesFor("marks", s); @@ -239,7 +225,10 @@ public void happy_happy() throws Exception { } try (var conn = postgresSqlDb.getDatabaseConnection(); - var pst = conn.prepareStatement("SELECT * FROM public.skills_question_attempts"); + var pst = conn.prepareStatement(""" + SELECT id, user_id, skill_assignment_id, skill_id, subskill_id, question->>'text' AS question_text, + question->>'answer' AS question_answer, question_attempt->>'result' AS result, marks, timestamp + FROM public.skills_question_attempts"""); var rs = pst.executeQuery()) { rs.next(); var p = new JSONObject(VALID_PAYLOAD); @@ -250,7 +239,7 @@ public void happy_happy() throws Exception { assertEquals(p.get("subskill_id").toString(), rs.getString("subskill_id")); assertEquals(p.getJSONObject("question").getString("text"), rs.getString("question_text")); assertEquals(p.getJSONObject("question").getString("answer"), rs.getString("question_answer")); - assertEquals(p.getString("question_attempt"), rs.getString("question_attempt")); + assertEquals(p.getJSONObject("question_attempt").getString("result"), rs.getString("result")); assertEquals(p.getInt("marks"), rs.getInt("marks")); assertThat(rs.getTimestamp("timestamp").toInstant()) .isCloseTo(p.getString("timestamp"), within(5, ChronoUnit.SECONDS)); @@ -283,7 +272,7 @@ private static String validPayload(final Consumer op) { .put("skill_id", SkillsFacadeIT.VALID_APP_ID) .put("subskill_id", 21) .put("question", new JSONObject().put("text", "2+2").put("answer", "4")) - .put("question_attempt", "4") + .put("question_attempt", new JSONObject().put("result", "4")) .put("marks", 1) .put("timestamp", Instant.now().toString()); op.accept(defaultPayload); From 914be9971e35ca80a9c93b1b4bfd9bd0be5bf572 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Thu, 18 Jun 2026 15:58:49 +0100 Subject: [PATCH 25/31] use skill attempt id as a nonce --- .../cam/cl/dtg/isaac/ISkillsAttemptManager.java | 4 +++- .../uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java | 6 ++++++ .../cl/dtg/isaac/api/managers/SkillsManager.java | 3 ++- .../cl/dtg/isaac/dao/PgSkillsAttemptManager.java | 4 +--- .../2026-06-skills_question_attempts.sql | 2 +- .../postgres-rutherford-create-script.sql | 2 +- .../ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 15 +++++++++++++-- 7 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/ISkillsAttemptManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/ISkillsAttemptManager.java index dcda860f05..f056f964da 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/ISkillsAttemptManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/ISkillsAttemptManager.java @@ -2,6 +2,8 @@ import uk.ac.cam.cl.dtg.isaac.dto.AnvilPayloadDTO; +import java.sql.SQLException; + public interface ISkillsAttemptManager { - void registerSkillsAttempt(final AnvilPayloadDTO attempt); + void registerSkillsAttempt(final AnvilPayloadDTO attempt) throws SQLException; } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java index 8a0e4e3530..be4459b9a0 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java @@ -19,6 +19,7 @@ import uk.ac.cam.cl.dtg.util.AbstractConfigLoader; import jakarta.servlet.http.HttpServletRequest; +import java.sql.SQLException; import jakarta.ws.rs.Consumes; import jakarta.ws.rs.POST; import jakarta.ws.rs.Path; @@ -87,6 +88,11 @@ public Response answerQuestion(@Context final HttpServletRequest request, return SegueErrorResponse.getNotLoggedInResponse(); } catch (final InvalidMarkingResponseException e) { return new SegueErrorResponse(Status.BAD_REQUEST, e.getMessage()).toResponse(); + } catch (final SQLException e) { + if ("23505".equals(e.getSQLState())) { + return new SegueErrorResponse(Status.CONFLICT, "Duplicate attempt ID").toResponse(); + } + return new SegueErrorResponse(Status.INTERNAL_SERVER_ERROR, "Something went wrong").toResponse(); } catch (final ContentManagerException e) { var error = new SegueErrorResponse(Status.NOT_FOUND, "Error locating the version requested", e); log.error(error.getErrorMessage(), e); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java index 631ed924e2..7f56ef5958 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java @@ -9,6 +9,7 @@ import uk.ac.cam.cl.dtg.segue.api.managers.UserAuthenticationManager; import uk.ac.cam.cl.dtg.util.AbstractConfigLoader; +import java.sql.SQLException; import java.util.Date; import static uk.ac.cam.cl.dtg.segue.api.Constants.*; @@ -83,7 +84,7 @@ public AnvilPayloadDTO parsePayload( * * @param attempt - the validated payload DTO */ - public void recordAttempt(final AnvilPayloadDTO attempt) { + public void recordAttempt(final AnvilPayloadDTO attempt) throws SQLException { skillsAttemptManager.registerSkillsAttempt(attempt); } } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java index 88e5b2cf79..df11f0fe03 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java @@ -17,7 +17,7 @@ public PgSkillsAttemptManager(final PostgresSqlDb database) { } @Override - public void registerSkillsAttempt(final AnvilPayloadDTO attempt) { + public void registerSkillsAttempt(final AnvilPayloadDTO attempt) throws SQLException { try (var conn = database.getDatabaseConnection(); var pst = conn.prepareStatement(""" INSERT INTO skills_question_attempts ( @@ -34,8 +34,6 @@ INSERT INTO skills_question_attempts ( pst.setInt(8, (Integer) attempt.getMarks()); pst.setTimestamp(9, new Timestamp(attempt.getTimestamp().getTime())); pst.executeUpdate(); - } catch (final SQLException e) { - throw new RuntimeException("Failed to record skills attempt", e); } } } diff --git a/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql b/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql index 5e6022a39e..4468047541 100644 --- a/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql +++ b/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql @@ -1,5 +1,5 @@ CREATE TABLE public.skills_question_attempts ( - id TEXT NOT NULL, + id TEXT PRIMARY KEY, user_id INTEGER NOT NULL, skill_assignment_id TEXT, skill_id TEXT NOT NULL, diff --git a/src/main/resources/db_scripts/postgres-rutherford-create-script.sql b/src/main/resources/db_scripts/postgres-rutherford-create-script.sql index 5daba22844..433d73dd1c 100644 --- a/src/main/resources/db_scripts/postgres-rutherford-create-script.sql +++ b/src/main/resources/db_scripts/postgres-rutherford-create-script.sql @@ -603,7 +603,7 @@ ALTER TABLE public.user_bookmarks OWNER TO rutherford; -- Name: skill_question_attempts; Type: TABLE; Schema: public; Owner: rutherford -- CREATE TABLE public.skills_question_attempts ( - id TEXT NOT NULL, + id TEXT PRIMARY KEY, user_id INTEGER NOT NULL, skill_assignment_id TEXT, skill_id TEXT NOT NULL, diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index bec0579877..1c6560adb3 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -36,7 +36,7 @@ public class SkillsFacadeIT extends IsaacIntegrationTestWithREST { private static final String HMAC_SHA_256 = "HmacSHA256"; private static final String VALID_URL = validUrl(); private static final String VALID_APP_ID = VALID_URL.split("/")[2]; - private static final String VALID_PAYLOAD = validPayload(p -> {}); + private static final String VALID_PAYLOAD = validPayload(null); private static final String VALID_HMAC = sign(HMAC_SECRET, VALID_PAYLOAD, HMAC_SHA_256); private static final JSONObject VALID_BODY = new JSONObject().put("payload", VALID_PAYLOAD).put("hmac", VALID_HMAC); @@ -210,6 +210,15 @@ private static void addNonEmptyCasesFor(final String fieldName, final Stream.Bui } } + @Test + public void duplicateAttemptId_Returns409() throws Exception { + var testP = validPayload(null); + var testBody = new JSONObject().put("payload", testP).put("hmac", sign(HMAC_SECRET, testP, HMAC_SHA_256)); + var client = testServer().client().loginAs(integrationTestUsers.TEST_STUDENT); + client.post(VALID_URL, testBody).readEntity(String.class); + client.post(VALID_URL, testBody).assertError("Duplicate attempt ID", Response.Status.CONFLICT); + } + @Test public void happy_happy() throws Exception { testServer().client() @@ -275,7 +284,9 @@ private static String validPayload(final Consumer op) { .put("question_attempt", new JSONObject().put("result", "4")) .put("marks", 1) .put("timestamp", Instant.now().toString()); - op.accept(defaultPayload); + if (op != null) { + op.accept(defaultPayload); + } return defaultPayload.toString(); } From 8eb05444193fad21febfca09ecc468ec1b69700a Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Thu, 18 Jun 2026 16:01:09 +0100 Subject: [PATCH 26/31] use UUID type for the id --- .../java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java | 2 +- .../db_scripts/migrations/2026-06-skills_question_attempts.sql | 2 +- .../resources/db_scripts/postgres-rutherford-create-script.sql | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java index df11f0fe03..591f164133 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java @@ -24,7 +24,7 @@ INSERT INTO skills_question_attempts ( id, user_id, skill_assignment_id, skill_id, subskill_id, question, question_attempt, marks, timestamp ) VALUES (?, ?, ?, ?, ?, (?::jsonb), (?::jsonb), ?, ?)""")) { - pst.setString(1, attempt.getId().toString()); + pst.setObject(1, attempt.getId()); pst.setLong(2, attempt.getUserId()); pst.setString(3, attempt.getSkillAssignmentId()); pst.setString(4, attempt.getSkillId()); diff --git a/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql b/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql index 4468047541..cb05c9cce0 100644 --- a/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql +++ b/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql @@ -1,5 +1,5 @@ CREATE TABLE public.skills_question_attempts ( - id TEXT PRIMARY KEY, + id UUID PRIMARY KEY, user_id INTEGER NOT NULL, skill_assignment_id TEXT, skill_id TEXT NOT NULL, diff --git a/src/main/resources/db_scripts/postgres-rutherford-create-script.sql b/src/main/resources/db_scripts/postgres-rutherford-create-script.sql index 433d73dd1c..abdf6a1f48 100644 --- a/src/main/resources/db_scripts/postgres-rutherford-create-script.sql +++ b/src/main/resources/db_scripts/postgres-rutherford-create-script.sql @@ -603,7 +603,7 @@ ALTER TABLE public.user_bookmarks OWNER TO rutherford; -- Name: skill_question_attempts; Type: TABLE; Schema: public; Owner: rutherford -- CREATE TABLE public.skills_question_attempts ( - id TEXT PRIMARY KEY, + id UUID PRIMARY KEY, user_id INTEGER NOT NULL, skill_assignment_id TEXT, skill_id TEXT NOT NULL, From f7fdb6192830ca569d7c22dd7f77a8a0e8858f36 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Thu, 18 Jun 2026 16:35:51 +0100 Subject: [PATCH 27/31] add foreign key constraint for user id --- .../2026-06-skills_question_attempts.sql | 4 ++- .../postgres-rutherford-create-script.sql | 34 +++++++++---------- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql b/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql index cb05c9cce0..6ed5c2b71b 100644 --- a/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql +++ b/src/main/resources/db_scripts/migrations/2026-06-skills_question_attempts.sql @@ -7,7 +7,9 @@ CREATE TABLE public.skills_question_attempts ( question JSONB NOT NULL, question_attempt JSONB NOT NULL, marks INTEGER NOT NULL, - timestamp TIMESTAMP WITH TIME ZONE NOT NULL + timestamp TIMESTAMP WITH TIME ZONE NOT NULL, + CONSTRAINT user_id_skills_question_attempts_fk FOREIGN KEY (user_id) REFERENCES public.users (id) + ON UPDATE CASCADE ON DELETE CASCADE -- account deletion does not delete user so this won't clear attempt either ); ALTER TABLE public.skills_question_attempts OWNER to rutherford; diff --git a/src/main/resources/db_scripts/postgres-rutherford-create-script.sql b/src/main/resources/db_scripts/postgres-rutherford-create-script.sql index abdf6a1f48..ca2c14f0c1 100644 --- a/src/main/resources/db_scripts/postgres-rutherford-create-script.sql +++ b/src/main/resources/db_scripts/postgres-rutherford-create-script.sql @@ -599,23 +599,6 @@ CREATE TABLE public.user_bookmarks ( ALTER TABLE public.user_bookmarks OWNER TO rutherford; --- --- Name: skill_question_attempts; Type: TABLE; Schema: public; Owner: rutherford --- -CREATE TABLE public.skills_question_attempts ( - id UUID PRIMARY KEY, - user_id INTEGER NOT NULL, - skill_assignment_id TEXT, - skill_id TEXT NOT NULL, - subskill_id TEXT NOT NULL, - question JSONB NOT NULL, - question_attempt JSONB NOT NULL, - marks INTEGER NOT NULL, - timestamp TIMESTAMP WITH TIME ZONE NOT NULL -); - -ALTER TABLE public.skills_question_attempts OWNER to rutherford; - -- -- Name: user_credentials; Type: TABLE; Schema: public; Owner: rutherford -- @@ -1559,7 +1542,24 @@ ALTER TABLE ONLY public.user_streak_freezes ALTER TABLE ONLY public.user_streak_targets ADD CONSTRAINT user_streak_targets_user_id_fkey FOREIGN KEY (user_id) REFERENCES public.users(id) ON DELETE CASCADE; +-- +-- Name: skill_question_attempts; Type: TABLE; Schema: public; Owner: rutherford +-- +CREATE TABLE public.skills_question_attempts ( + id UUID PRIMARY KEY, + user_id INTEGER NOT NULL, + skill_assignment_id TEXT, + skill_id TEXT NOT NULL, + subskill_id TEXT NOT NULL, + question JSONB NOT NULL, + question_attempt JSONB NOT NULL, + marks INTEGER NOT NULL, + timestamp TIMESTAMP WITH TIME ZONE NOT NULL, + CONSTRAINT user_id_skills_question_attempts_fk FOREIGN KEY (user_id) REFERENCES public.users (id) + ON UPDATE CASCADE ON DELETE CASCADE -- account deletion does not delete user so this won't clear attempt either +); +ALTER TABLE public.skills_question_attempts OWNER to rutherford; -- -- PostgreSQL database dump complete -- From 298816e9f9b72081d7cc6f609909f9c6aac62395 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Thu, 18 Jun 2026 17:07:58 +0100 Subject: [PATCH 28/31] endpoint echoes back dto for consistency with how the questions endpoint works --- .../ac/cam/cl/dtg/isaac/api/SkillsFacade.java | 4 +- .../dtg/isaac/dao/PgSkillsAttemptManager.java | 4 +- .../cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java | 11 +++-- .../cam/cl/dtg/isaac/api/SkillsFacadeIT.java | 46 ++++++++++++------- 4 files changed, 42 insertions(+), 23 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java index be4459b9a0..7ee1d0c2fe 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java @@ -22,6 +22,7 @@ import java.sql.SQLException; import jakarta.ws.rs.Consumes; import jakarta.ws.rs.POST; +import jakarta.ws.rs.Produces; import jakarta.ws.rs.Path; import jakarta.ws.rs.PathParam; import jakarta.ws.rs.core.Context; @@ -65,6 +66,7 @@ public SkillsFacade(final AbstractConfigLoader properties, final UserAccountMana @POST @Path("/{appId}/answer") @Consumes(MediaType.APPLICATION_JSON) + @Produces(MediaType.APPLICATION_JSON) public Response answerQuestion(@Context final HttpServletRequest request, @PathParam("appId") final String appId, final AnvilMarkingResponseDTO markingResponse) { @@ -83,7 +85,7 @@ public Response answerQuestion(@Context final HttpServletRequest request, var payloadDTO = skillsManager.parsePayload(markingResponse.getPayload(), currentUser.getId(), appId); skillsManager.recordAttempt(payloadDTO); - return Response.ok().build(); + return Response.ok(payloadDTO).build(); } catch (final NoUserLoggedInException e) { return SegueErrorResponse.getNotLoggedInResponse(); } catch (final InvalidMarkingResponseException e) { diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java index 591f164133..5de91669f4 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java @@ -29,8 +29,8 @@ INSERT INTO skills_question_attempts ( pst.setString(3, attempt.getSkillAssignmentId()); pst.setString(4, attempt.getSkillId()); pst.setString(5, attempt.getSubskillId()); - pst.setString(6, attempt.getQuestion()); - pst.setString(7, attempt.getQuestionAttempt()); + pst.setString(6, attempt.getQuestion().toString()); + pst.setString(7, attempt.getQuestionAttempt().toString()); pst.setInt(8, (Integer) attempt.getMarks()); pst.setTimestamp(9, new Timestamp(attempt.getTimestamp().getTime())); pst.executeUpdate(); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java index e46103e1c5..20a64973fe 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilPayloadDTO.java @@ -5,6 +5,8 @@ import com.fasterxml.jackson.annotation.JsonSetter; import com.fasterxml.jackson.annotation.Nulls; import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.PropertyNamingStrategies; +import com.fasterxml.jackson.databind.annotation.JsonNaming; import org.apache.commons.lang3.Validate; import java.util.Date; @@ -14,6 +16,7 @@ /** * DTO representing the signed payload content from the external Anvil marking server. */ +@JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class) public class AnvilPayloadDTO { private final UUID id; private final Long userId; @@ -88,12 +91,12 @@ public String getSubskillId() { return subskillId; } - public String getQuestion() { - return question.toString(); + public JsonNode getQuestion() { + return question; } - public String getQuestionAttempt() { - return questionAttempt.toString(); + public JsonNode getQuestionAttempt() { + return questionAttempt; } public Number getMarks() { diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java index 1c6560adb3..a4114d9168 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacadeIT.java @@ -26,6 +26,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.within; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; import static uk.ac.cam.cl.dtg.isaac.api.ITConstants.REGRESSION_TEST_PAGE_ID; import static uk.ac.cam.cl.dtg.isaac.api.ITConstants.TEST_STUDENT_ID; @@ -221,11 +222,13 @@ public void duplicateAttemptId_Returns409() throws Exception { @Test public void happy_happy() throws Exception { - testServer().client() + var expected = new JSONObject(VALID_PAYLOAD); + var actualReq = testServer().client() .loginAs(integrationTestUsers.TEST_STUDENT) .post(VALID_URL, VALID_BODY) - .readEntity(String.class); + .readEntityAsJson(); + // assert there is exactly 1 row in the database try (var conn = postgresSqlDb.getDatabaseConnection(); var pst = conn.prepareStatement("SELECT COUNT(*) FROM public.skills_question_attempts"); var rs = pst.executeQuery()) { @@ -233,26 +236,37 @@ public void happy_happy() throws Exception { assertEquals(1, rs.getInt(1)); } + // assert everything has been recorded to the database try (var conn = postgresSqlDb.getDatabaseConnection(); var pst = conn.prepareStatement(""" SELECT id, user_id, skill_assignment_id, skill_id, subskill_id, question->>'text' AS question_text, question->>'answer' AS question_answer, question_attempt->>'result' AS result, marks, timestamp FROM public.skills_question_attempts"""); - var rs = pst.executeQuery()) { - rs.next(); - var p = new JSONObject(VALID_PAYLOAD); - assertEquals(p.getString("id"), rs.getString("id")); - assertEquals(p.getInt("user_id"), rs.getInt("user_id")); - assertNull(rs.getString("skill_assignment_id")); - assertEquals(p.getString("skill_id"), rs.getString("skill_id")); - assertEquals(p.get("subskill_id").toString(), rs.getString("subskill_id")); - assertEquals(p.getJSONObject("question").getString("text"), rs.getString("question_text")); - assertEquals(p.getJSONObject("question").getString("answer"), rs.getString("question_answer")); - assertEquals(p.getJSONObject("question_attempt").getString("result"), rs.getString("result")); - assertEquals(p.getInt("marks"), rs.getInt("marks")); - assertThat(rs.getTimestamp("timestamp").toInstant()) - .isCloseTo(p.getString("timestamp"), within(5, ChronoUnit.SECONDS)); + var actualDb = pst.executeQuery()) { + actualDb.next(); + assertEquals(expected.getString("id"), actualDb.getString("id")); + assertEquals(expected.getInt("user_id"), actualDb.getInt("user_id")); + assertNull(actualDb.getString("skill_assignment_id")); + assertEquals(expected.getString("skill_id"), actualDb.getString("skill_id")); + assertEquals(expected.get("subskill_id").toString(), actualDb.getString("subskill_id")); + assertEquals(expected.getJSONObject("question").getString("text"), actualDb.getString("question_text")); + assertEquals(expected.getJSONObject("question").getString("answer"), actualDb.getString("question_answer")); + assertEquals(expected.getJSONObject("question_attempt").getString("result"), actualDb.getString("result")); + assertEquals(expected.getInt("marks"), actualDb.getInt("marks")); + assertThat(actualDb.getTimestamp("timestamp").toInstant()) + .isCloseTo(expected.getString("timestamp"), within(5, ChronoUnit.SECONDS)); } + + // assert the request echoes back the payload + assertEquals(expected.getString("id"), actualReq.getString("id")); + assertEquals(expected.getInt("user_id"), actualReq.getInt("user_id")); + assertEquals(expected.getString("skill_id"), actualReq.getString("skill_id")); + assertEquals(expected.get("subskill_id").toString(), actualReq.getString("subskill_id")); + assertEquals(expected.getJSONObject("question").toString(), actualReq.getJSONObject("question").toString()); + assertEquals(expected.getJSONObject("question_attempt").toString(), + actualReq.getJSONObject("question_attempt").toString()); + assertEquals(expected.getInt("marks"), actualReq.getInt("marks")); + assertFalse(actualReq.has("skill_assignment_id")); } private static GitContentManager brokenContentManager() throws Exception { From b230adcf5d28e718af4afc3c7b853616b3996120 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Thu, 18 Jun 2026 17:28:34 +0100 Subject: [PATCH 29/31] fix checkstyle problems --- .../java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java | 4 ++-- .../ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java | 3 +-- .../ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java | 3 ++- .../cl/dtg/isaac/{ => quiz}/ISkillsAttemptManager.java | 3 ++- .../configuration/SegueGuiceConfigurationModule.java | 2 +- .../ac/cam/cl/dtg/isaac/api/ElasticSearchTestHelper.java | 9 ++++++++- .../cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java | 4 +++- 7 files changed, 19 insertions(+), 9 deletions(-) rename src/main/java/uk/ac/cam/cl/dtg/isaac/{ => quiz}/ISkillsAttemptManager.java (65%) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java index 7ee1d0c2fe..76277f88db 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java @@ -19,16 +19,16 @@ import uk.ac.cam.cl.dtg.util.AbstractConfigLoader; import jakarta.servlet.http.HttpServletRequest; -import java.sql.SQLException; import jakarta.ws.rs.Consumes; import jakarta.ws.rs.POST; -import jakarta.ws.rs.Produces; import jakarta.ws.rs.Path; import jakarta.ws.rs.PathParam; +import jakarta.ws.rs.Produces; import jakarta.ws.rs.core.Context; import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.core.Response; import jakarta.ws.rs.core.Response.Status; +import java.sql.SQLException; /** * Skills Facade, supports interaction related to the Isaac Skill Practice apps. diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java index 7f56ef5958..1546ee5bf7 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java @@ -3,9 +3,9 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.inject.Inject; -import uk.ac.cam.cl.dtg.isaac.ISkillsAttemptManager; import uk.ac.cam.cl.dtg.isaac.dto.AnvilMarkingResponseDTO; import uk.ac.cam.cl.dtg.isaac.dto.AnvilPayloadDTO; +import uk.ac.cam.cl.dtg.isaac.quiz.ISkillsAttemptManager; import uk.ac.cam.cl.dtg.segue.api.managers.UserAuthenticationManager; import uk.ac.cam.cl.dtg.util.AbstractConfigLoader; @@ -14,7 +14,6 @@ import static uk.ac.cam.cl.dtg.segue.api.Constants.*; - /** * Manager for Isaac Skills Practice app interactions. */ diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java index 5de91669f4..3b3801feda 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java @@ -1,13 +1,14 @@ package uk.ac.cam.cl.dtg.isaac.dao; import com.google.inject.Inject; -import uk.ac.cam.cl.dtg.isaac.ISkillsAttemptManager; import uk.ac.cam.cl.dtg.isaac.dto.AnvilPayloadDTO; +import uk.ac.cam.cl.dtg.isaac.quiz.ISkillsAttemptManager; import uk.ac.cam.cl.dtg.segue.database.PostgresSqlDb; import java.sql.SQLException; import java.sql.Timestamp; +/** PostgreSQL-backed persistence for Anvil skills question attempts. */ public class PgSkillsAttemptManager implements ISkillsAttemptManager { private final PostgresSqlDb database; diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/ISkillsAttemptManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/ISkillsAttemptManager.java similarity index 65% rename from src/main/java/uk/ac/cam/cl/dtg/isaac/ISkillsAttemptManager.java rename to src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/ISkillsAttemptManager.java index f056f964da..46927a95ae 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/ISkillsAttemptManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/ISkillsAttemptManager.java @@ -1,9 +1,10 @@ -package uk.ac.cam.cl.dtg.isaac; +package uk.ac.cam.cl.dtg.isaac.quiz; import uk.ac.cam.cl.dtg.isaac.dto.AnvilPayloadDTO; import java.sql.SQLException; +/** Persistence interface for recording Anvil skills question attempts. */ public interface ISkillsAttemptManager { void registerSkillsAttempt(final AnvilPayloadDTO attempt) throws SQLException; } diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/configuration/SegueGuiceConfigurationModule.java b/src/main/java/uk/ac/cam/cl/dtg/segue/configuration/SegueGuiceConfigurationModule.java index 17d11e680b..67fb5dd377 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/configuration/SegueGuiceConfigurationModule.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/configuration/SegueGuiceConfigurationModule.java @@ -38,7 +38,7 @@ import org.reflections.Reflections; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import uk.ac.cam.cl.dtg.isaac.ISkillsAttemptManager; +import uk.ac.cam.cl.dtg.isaac.quiz.ISkillsAttemptManager; import uk.ac.cam.cl.dtg.isaac.api.managers.AssignmentManager; import uk.ac.cam.cl.dtg.isaac.api.managers.GameManager; import uk.ac.cam.cl.dtg.isaac.api.managers.QuizAssignmentManager; diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/ElasticSearchTestHelper.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/ElasticSearchTestHelper.java index 66d8998a23..964d8097e3 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/ElasticSearchTestHelper.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/ElasticSearchTestHelper.java @@ -10,12 +10,17 @@ import static org.testcontainers.shaded.com.google.common.collect.Maps.immutableEntry; -@SuppressWarnings("checkstyle:MissingJavadocType") +/** + * Test utility for indexing content directly into Elasticsearch, bypassing the Git content pipeline. + * Use instead of text fixtures for more self-contained tests and to avoid needing to use the same + * database state across all tests. + * */ public class ElasticSearchTestHelper { private final ElasticSearchIndexer elasticSearchProvider; private final GitContentManager contentManager; private final ContentSubclassMapper contentMapper; + /** Constructor.*/ public ElasticSearchTestHelper(final ElasticSearchIndexer elasticSearchProvider, final GitContentManager contentManager, final ContentSubclassMapper contentMapper) { @@ -24,6 +29,7 @@ public ElasticSearchTestHelper(final ElasticSearchIndexer elasticSearchProvider, this.contentMapper = contentMapper; } + /** Indexes a typed content object and returns it. */ public T persist(final T content) throws Exception { elasticSearchProvider.bulkIndexWithIDs( contentManager.getCurrentContentSHA(), @@ -35,6 +41,7 @@ public T persist(final T content) throws Exception { return content; } + /** Indexes a raw JSON content object with a fixed ID of {@code "i1"} and returns it. */ public JSONObject persistJSON(final JSONObject contentJSON) throws Exception { contentJSON.put("id", "i1"); elasticSearchProvider.bulkIndexWithIDs( diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java index df3d323b3a..8608e91da8 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java @@ -172,7 +172,9 @@ public TestResponse post(final String url, final Object body) { } public TestClient loginAs(final RegisteredUser user) { - Invocation.Builder request = client.target(baseUrl + "/auth/SEGUE/authenticate").request(MediaType.APPLICATION_JSON); + Invocation.Builder request = client + .target(baseUrl + "/auth/SEGUE/authenticate") + .request(MediaType.APPLICATION_JSON); LocalAuthDTO body = new LocalAuthDTO(); body.setEmail(user.getEmail()); body.setPassword("test1234"); From 8bf13c75f6394bfe320b950a0dfbf4fbfa3dfbae Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Fri, 19 Jun 2026 14:35:45 +0100 Subject: [PATCH 30/31] only allow endpoint if HMAC_SECRET is set --- .../cl/dtg/isaac/configuration/IsaacApplicationRegister.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/configuration/IsaacApplicationRegister.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/configuration/IsaacApplicationRegister.java index 9d9bce7ffd..5bcee613a1 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/configuration/IsaacApplicationRegister.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/configuration/IsaacApplicationRegister.java @@ -124,7 +124,9 @@ public final Set getSingletons() { this.singletons.add(injector.getInstance(EmailFacade.class)); this.singletons.add(injector.getInstance(QuizFacade.class)); this.singletons.add(injector.getInstance(BookmarksFacade.class)); - this.singletons.add(injector.getInstance(SkillsFacade.class)); + if (injector.getInstance(AbstractConfigLoader.class).getProperty(SKILLS_HMAC_SECRET) != null) { + this.singletons.add(injector.getInstance(SkillsFacade.class)); + } // initialise filters this.singletons.add(injector.getInstance(PerformanceMonitor.class)); From 419192219918ab13b01598f124a8a8bc2962d9d5 Mon Sep 17 00:00:00 2001 From: Barna Magyarkuti Date: Fri, 19 Jun 2026 15:20:19 +0100 Subject: [PATCH 31/31] add logging, minor refactors --- .../ac/cam/cl/dtg/isaac/api/SkillsFacade.java | 41 +++++++++++-------- .../InvalidAnvilMarkingRequestException.java | 19 +++++++++ .../InvalidMarkingResponseException.java | 13 ------ .../dtg/isaac/api/managers/SkillsManager.java | 26 ++++++------ .../dtg/isaac/dao/PgSkillsAttemptManager.java | 8 +++- ...seDTO.java => AnvilMarkingRequestDTO.java} | 6 +-- .../dtg/isaac/quiz/ISkillsAttemptManager.java | 5 +-- 7 files changed, 69 insertions(+), 49 deletions(-) create mode 100644 src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/InvalidAnvilMarkingRequestException.java delete mode 100644 src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/InvalidMarkingResponseException.java rename src/main/java/uk/ac/cam/cl/dtg/isaac/dto/{AnvilMarkingResponseDTO.java => AnvilMarkingRequestDTO.java} (85%) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java index 76277f88db..951cfafced 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/SkillsFacade.java @@ -4,16 +4,18 @@ import io.swagger.v3.oas.annotations.tags.Tag; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import uk.ac.cam.cl.dtg.isaac.api.managers.InvalidMarkingResponseException; +import uk.ac.cam.cl.dtg.isaac.api.managers.InvalidAnvilMarkingRequestException; import uk.ac.cam.cl.dtg.isaac.api.managers.SkillsManager; import uk.ac.cam.cl.dtg.isaac.dos.content.AnvilApp; import uk.ac.cam.cl.dtg.isaac.dos.content.Content; -import uk.ac.cam.cl.dtg.isaac.dto.AnvilMarkingResponseDTO; +import uk.ac.cam.cl.dtg.isaac.dto.AnvilMarkingRequestDTO; +import uk.ac.cam.cl.dtg.isaac.dto.AnvilPayloadDTO; import uk.ac.cam.cl.dtg.isaac.dto.SegueErrorResponse; import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; import uk.ac.cam.cl.dtg.segue.api.managers.UserAccountManager; import uk.ac.cam.cl.dtg.segue.auth.exceptions.NoUserLoggedInException; import uk.ac.cam.cl.dtg.segue.dao.ILogManager; +import uk.ac.cam.cl.dtg.segue.dao.SegueDatabaseException; import uk.ac.cam.cl.dtg.segue.dao.content.ContentManagerException; import uk.ac.cam.cl.dtg.segue.dao.content.GitContentManager; import uk.ac.cam.cl.dtg.util.AbstractConfigLoader; @@ -28,11 +30,8 @@ import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.core.Response; import jakarta.ws.rs.core.Response.Status; -import java.sql.SQLException; -/** - * Skills Facade, supports interaction related to the Isaac Skill Practice apps. - */ +/** Skills Facade, supports interaction related to the Isaac Skill Practice apps. */ @Path("/skills") @Tag(name = "SkillsFacade", description = "/skills") public class SkillsFacade extends AbstractIsaacFacade { @@ -69,32 +68,42 @@ public SkillsFacade(final AbstractConfigLoader properties, final UserAccountMana @Produces(MediaType.APPLICATION_JSON) public Response answerQuestion(@Context final HttpServletRequest request, @PathParam("appId") final String appId, - final AnvilMarkingResponseDTO markingResponse) { + final AnvilMarkingRequestDTO markingRequest) { try { RegisteredUserDTO currentUser = userManager.getCurrentRegisteredUser(request); + if (!(hasAnvilApp(this.contentManager.getContentDOById(appId)))) { var error = new SegueErrorResponse(Status.NOT_FOUND, "No app found for given id: " + appId); log.warn(error.getErrorMessage()); return error.toResponse(); } - if (!skillsManager.isHmacValid(markingResponse)) { - return new SegueErrorResponse(Status.BAD_REQUEST, "Invalid HMAC signature").toResponse(); + if (!skillsManager.isHmacValid(markingRequest)) { + var error = new SegueErrorResponse(Status.BAD_REQUEST, "Invalid HMAC signature"); + log.warn(error.getErrorMessage()); + return error.toResponse(); } - var payloadDTO = skillsManager.parsePayload(markingResponse.getPayload(), currentUser.getId(), appId); + AnvilPayloadDTO payloadDTO = skillsManager.parsePayload( + markingRequest.getPayload(), currentUser.getId(), appId); skillsManager.recordAttempt(payloadDTO); return Response.ok(payloadDTO).build(); } catch (final NoUserLoggedInException e) { return SegueErrorResponse.getNotLoggedInResponse(); - } catch (final InvalidMarkingResponseException e) { - return new SegueErrorResponse(Status.BAD_REQUEST, e.getMessage()).toResponse(); - } catch (final SQLException e) { - if ("23505".equals(e.getSQLState())) { - return new SegueErrorResponse(Status.CONFLICT, "Duplicate attempt ID").toResponse(); + } catch (final InvalidAnvilMarkingRequestException e) { + var error = new SegueErrorResponse(Status.BAD_REQUEST, e.getMessage()); + log.warn(error.getErrorMessage() + ", " + e.getDetailedProblem()); + return error.toResponse(); + } catch (final SegueDatabaseException e) { + if ("Duplicate attempt ID".equals(e.getMessage())) { + var error = new SegueErrorResponse(Status.CONFLICT, "Duplicate attempt ID"); + log.warn(error.getErrorMessage()); + return error.toResponse(); } - return new SegueErrorResponse(Status.INTERNAL_SERVER_ERROR, "Something went wrong").toResponse(); + var error = new SegueErrorResponse(Status.INTERNAL_SERVER_ERROR, "Something went wrong"); + log.error(error.getErrorMessage()); + return error.toResponse(); } catch (final ContentManagerException e) { var error = new SegueErrorResponse(Status.NOT_FOUND, "Error locating the version requested", e); log.error(error.getErrorMessage(), e); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/InvalidAnvilMarkingRequestException.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/InvalidAnvilMarkingRequestException.java new file mode 100644 index 0000000000..6be145d8f5 --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/InvalidAnvilMarkingRequestException.java @@ -0,0 +1,19 @@ +package uk.ac.cam.cl.dtg.isaac.api.managers; + +/** + * An exception that indicates an invalid marking response was received from the external marking server. + */ +public class InvalidAnvilMarkingRequestException extends Exception { + private final String detailedProblem; + + /** Constructor with message, detailed problem.*/ + public InvalidAnvilMarkingRequestException(final String message, final String detailedProblem) { + super(message); + this.detailedProblem = detailedProblem; + } + + /** Get detailed problem.*/ + public String getDetailedProblem() { + return detailedProblem; + } +} diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/InvalidMarkingResponseException.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/InvalidMarkingResponseException.java deleted file mode 100644 index e234c462c3..0000000000 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/InvalidMarkingResponseException.java +++ /dev/null @@ -1,13 +0,0 @@ -package uk.ac.cam.cl.dtg.isaac.api.managers; - -/** - * An exception that indicates an invalid marking response was received from the external marking server. - */ -public class InvalidMarkingResponseException extends Exception { - /** - * Constructor with message. - */ - public InvalidMarkingResponseException(final String message) { - super(message); - } -} diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java index 1546ee5bf7..352e367829 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/SkillsManager.java @@ -3,13 +3,13 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.inject.Inject; -import uk.ac.cam.cl.dtg.isaac.dto.AnvilMarkingResponseDTO; +import uk.ac.cam.cl.dtg.isaac.dto.AnvilMarkingRequestDTO; import uk.ac.cam.cl.dtg.isaac.dto.AnvilPayloadDTO; import uk.ac.cam.cl.dtg.isaac.quiz.ISkillsAttemptManager; import uk.ac.cam.cl.dtg.segue.api.managers.UserAuthenticationManager; +import uk.ac.cam.cl.dtg.segue.dao.SegueDatabaseException; import uk.ac.cam.cl.dtg.util.AbstractConfigLoader; -import java.sql.SQLException; import java.util.Date; import static uk.ac.cam.cl.dtg.segue.api.Constants.*; @@ -38,10 +38,10 @@ public SkillsManager(final AbstractConfigLoader properties, final ISkillsAttempt /** * Verifies that the HMAC in the DTO matches the expected signature of the payload. * - * @param dto - the parsed marking response + * @param dto - the parsed marking request * @return whether the hmac was valid */ - public boolean isHmacValid(final AnvilMarkingResponseDTO dto) { + public boolean isHmacValid(final AnvilMarkingRequestDTO dto) { String expected = UserAuthenticationManager.calculateHMAC(hmacSecret, dto.getPayload()); return expected.equals(dto.getHmac()); } @@ -49,32 +49,32 @@ public boolean isHmacValid(final AnvilMarkingResponseDTO dto) { /** * Validates the content of the signed payload string. * - * @param payloadStr - the payload string from the marking response + * @param payloadStr - the payload string from the marking request * @param userId - the ID of the currently authenticated user * @param appId - the app ID from the URL, which must match the payload's skill_id - * @throws InvalidMarkingResponseException if the payload is malformed or any validation fails + * @throws InvalidAnvilMarkingRequestException if the payload is malformed or any validation fails */ public AnvilPayloadDTO parsePayload( final String payloadStr, final long userId, final String appId - ) throws InvalidMarkingResponseException { + ) throws InvalidAnvilMarkingRequestException { try { if (payloadStr.length() > 10 * 1024) { - throw new InvalidMarkingResponseException("Payload too large"); + throw new InvalidAnvilMarkingRequestException("Payload too large", null); } AnvilPayloadDTO dto = objectMapper.readValue(payloadStr, AnvilPayloadDTO.class); if (dto.getUserId() != userId) { - throw new InvalidMarkingResponseException("Payload user_id does not match session"); + throw new InvalidAnvilMarkingRequestException("Payload user_id does not match session", null); } if (dto.getTimestamp().before(new Date(System.currentTimeMillis() - 300_000L))) { - throw new InvalidMarkingResponseException("Payload timestamp is outside the allowed window"); + throw new InvalidAnvilMarkingRequestException("Payload timestamp is outside the allowed window", null); } if (!dto.getSkillId().equals(appId)) { - throw new InvalidMarkingResponseException("Payload skill_id does not match app"); + throw new InvalidAnvilMarkingRequestException("Payload skill_id does not match app", null); } return dto; } catch (final JsonProcessingException e) { - throw new InvalidMarkingResponseException("Invalid payload"); + throw new InvalidAnvilMarkingRequestException("Invalid payload", e.getMessage()); } } @@ -83,7 +83,7 @@ public AnvilPayloadDTO parsePayload( * * @param attempt - the validated payload DTO */ - public void recordAttempt(final AnvilPayloadDTO attempt) throws SQLException { + public void recordAttempt(final AnvilPayloadDTO attempt) throws SegueDatabaseException { skillsAttemptManager.registerSkillsAttempt(attempt); } } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java index 3b3801feda..540343074a 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgSkillsAttemptManager.java @@ -3,6 +3,7 @@ import com.google.inject.Inject; import uk.ac.cam.cl.dtg.isaac.dto.AnvilPayloadDTO; import uk.ac.cam.cl.dtg.isaac.quiz.ISkillsAttemptManager; +import uk.ac.cam.cl.dtg.segue.dao.SegueDatabaseException; import uk.ac.cam.cl.dtg.segue.database.PostgresSqlDb; import java.sql.SQLException; @@ -18,7 +19,7 @@ public PgSkillsAttemptManager(final PostgresSqlDb database) { } @Override - public void registerSkillsAttempt(final AnvilPayloadDTO attempt) throws SQLException { + public void registerSkillsAttempt(final AnvilPayloadDTO attempt) throws SegueDatabaseException { try (var conn = database.getDatabaseConnection(); var pst = conn.prepareStatement(""" INSERT INTO skills_question_attempts ( @@ -35,6 +36,11 @@ INSERT INTO skills_question_attempts ( pst.setInt(8, (Integer) attempt.getMarks()); pst.setTimestamp(9, new Timestamp(attempt.getTimestamp().getTime())); pst.executeUpdate(); + } catch (final SQLException e) { + if ("23505".equals(e.getSQLState())) { + throw new SegueDatabaseException("Duplicate attempt ID"); + } + throw new SegueDatabaseException("Something went wrong saving the attempt."); } } } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilMarkingResponseDTO.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilMarkingRequestDTO.java similarity index 85% rename from src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilMarkingResponseDTO.java rename to src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilMarkingRequestDTO.java index a73ba2d8f3..cee248527a 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilMarkingResponseDTO.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/AnvilMarkingRequestDTO.java @@ -4,9 +4,9 @@ import com.fasterxml.jackson.annotation.JsonProperty; /** - * DTO representing the response received from the external Anvil marking server. + * DTO representing the request received from the external Anvil marking server. */ -public class AnvilMarkingResponseDTO { +public class AnvilMarkingRequestDTO { private final String payload; private final String hmac; @@ -17,7 +17,7 @@ public class AnvilMarkingResponseDTO { * @param hmac - the HMAC-SHA256 hex digest authenticating the payload */ @JsonCreator - public AnvilMarkingResponseDTO( + public AnvilMarkingRequestDTO( @JsonProperty(value = "payload", required = true) final String payload, @JsonProperty(value = "hmac", required = true) final String hmac) { this.payload = payload; diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/ISkillsAttemptManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/ISkillsAttemptManager.java index 46927a95ae..1d703af4a9 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/ISkillsAttemptManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/ISkillsAttemptManager.java @@ -1,10 +1,9 @@ package uk.ac.cam.cl.dtg.isaac.quiz; import uk.ac.cam.cl.dtg.isaac.dto.AnvilPayloadDTO; - -import java.sql.SQLException; +import uk.ac.cam.cl.dtg.segue.dao.SegueDatabaseException; /** Persistence interface for recording Anvil skills question attempts. */ public interface ISkillsAttemptManager { - void registerSkillsAttempt(final AnvilPayloadDTO attempt) throws SQLException; + void registerSkillsAttempt(final AnvilPayloadDTO attempt) throws SegueDatabaseException; }