From 82f32da8e86bc1d89568a2546393101f9d405f6f Mon Sep 17 00:00:00 2001 From: Isaries Date: Thu, 16 Jul 2026 17:02:19 +0800 Subject: [PATCH 1/3] fix(security): only remove a student from a run The remove student endpoint took the target user straight from the student id and never checked it was a student. Both steps of removeStudentFromRun are already scoped to the run, so passing the id of a user outside the run does nothing. A teacher is a different matter: getListByRunAndUser does not filter by role and teachers do have a workgroup in a run, so passing the id of a teacher who shares the run removed that teacher from their own workgroup in it. Check that the target is a student before removing them. --- .../RemoveStudentRunController.java | 3 + .../RemoveStudentRunControllerTest.java | 88 +++++++++++++++++++ 2 files changed, 91 insertions(+) create mode 100644 src/test/java/org/wise/portal/presentation/web/controllers/teacher/management/RemoveStudentRunControllerTest.java diff --git a/src/main/java/org/wise/portal/presentation/web/controllers/teacher/management/RemoveStudentRunController.java b/src/main/java/org/wise/portal/presentation/web/controllers/teacher/management/RemoveStudentRunController.java index 04133934e..751508462 100644 --- a/src/main/java/org/wise/portal/presentation/web/controllers/teacher/management/RemoveStudentRunController.java +++ b/src/main/java/org/wise/portal/presentation/web/controllers/teacher/management/RemoveStudentRunController.java @@ -33,6 +33,9 @@ public void removeStudent(Authentication auth, @PathVariable Long runId, Run run = runService.retrieveById(runId); if (runService.hasWritePermission(auth, run)) { User studentUser = userService.retrieveById(studentId); + if (!studentUser.isStudent()) { + throw new AccessDeniedException("User does not have permission to remove this user"); + } studentService.removeStudentFromRun(studentUser, run); } else { throw new AccessDeniedException("User does not have permission to remove student from run"); diff --git a/src/test/java/org/wise/portal/presentation/web/controllers/teacher/management/RemoveStudentRunControllerTest.java b/src/test/java/org/wise/portal/presentation/web/controllers/teacher/management/RemoveStudentRunControllerTest.java new file mode 100644 index 000000000..ea4f36343 --- /dev/null +++ b/src/test/java/org/wise/portal/presentation/web/controllers/teacher/management/RemoveStudentRunControllerTest.java @@ -0,0 +1,88 @@ +package org.wise.portal.presentation.web.controllers.teacher.management; + +import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.replay; +import static org.easymock.EasyMock.verify; +import static org.junit.Assert.fail; + +import org.easymock.EasyMockExtension; +import org.easymock.Mock; +import org.easymock.TestSubject; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.springframework.security.access.AccessDeniedException; +import org.wise.portal.presentation.web.controllers.APIControllerTest; +import org.wise.portal.service.student.StudentService; + +@ExtendWith(EasyMockExtension.class) +public class RemoveStudentRunControllerTest extends APIControllerTest { + + @TestSubject + private RemoveStudentRunController controller = new RemoveStudentRunController(); + + @Mock + private StudentService studentService; + + private void replayServices() { + replay(runService, studentService, userService); + } + + private void verifyServices() { + verify(runService, studentService, userService); + } + + @Test + public void removeStudent_NoWritePermission_ThrowAccessDenied() throws Exception { + expect(runService.retrieveById(runId1)).andReturn(run1); + expect(runService.hasWritePermission(teacherAuth, run1)).andReturn(false); + replayServices(); + try { + controller.removeStudent(teacherAuth, runId1, student1Id); + fail("Expected AccessDeniedException to be thrown"); + } catch (AccessDeniedException e) { + } + verifyServices(); + } + + @Test + public void removeStudent_TargetUserIsTeacher_ThrowAccessDenied() throws Exception { + expect(runService.retrieveById(runId1)).andReturn(run1); + expect(runService.hasWritePermission(teacherAuth, run1)).andReturn(true); + expect(userService.retrieveById(teacher2Id)).andReturn(teacher2); + replayServices(); + try { + controller.removeStudent(teacherAuth, runId1, teacher2Id); + fail("Expected AccessDeniedException to be thrown"); + } catch (AccessDeniedException e) { + } + verifyServices(); + } + + @Test + public void removeStudent_Student_RemoveStudentFromRun() throws Exception { + expect(runService.retrieveById(runId1)).andReturn(run1); + expect(runService.hasWritePermission(teacherAuth, run1)).andReturn(true); + expect(userService.retrieveById(student1Id)).andReturn(student1); + studentService.removeStudentFromRun(student1, run1); + replayServices(); + controller.removeStudent(teacherAuth, runId1, student1Id); + verifyServices(); + } + + /** + * Both steps of removeStudentFromRun are scoped to the run, so a student who is not in the run + * is left untouched. Rejecting them here instead would turn a repeated removal into an error, + * and would leave a student who is still in a workgroup but no longer in a period with no way + * to be cleaned up. + */ + @Test + public void removeStudent_StudentNotInRun_RemoveStudentFromRun() throws Exception { + expect(runService.retrieveById(runId1)).andReturn(run1); + expect(runService.hasWritePermission(teacherAuth, run1)).andReturn(true); + expect(userService.retrieveById(student2Id)).andReturn(student2); + studentService.removeStudentFromRun(student2, run1); + replayServices(); + controller.removeStudent(teacherAuth, runId1, student2Id); + verifyServices(); + } +} From d0a3a5e4948cc17f5c63178aaa809a533e593366 Mon Sep 17 00:00:00 2001 From: Leo <154660927+Isaries@users.noreply.github.com> Date: Fri, 31 Jul 2026 20:57:37 +0800 Subject: [PATCH 2/3] Update src/test/java/org/wise/portal/presentation/web/controllers/teacher/management/RemoveStudentRunControllerTest.java Co-authored-by: Aaron Detre --- .../teacher/management/RemoveStudentRunControllerTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/wise/portal/presentation/web/controllers/teacher/management/RemoveStudentRunControllerTest.java b/src/test/java/org/wise/portal/presentation/web/controllers/teacher/management/RemoveStudentRunControllerTest.java index ea4f36343..f6e87b2ce 100644 --- a/src/test/java/org/wise/portal/presentation/web/controllers/teacher/management/RemoveStudentRunControllerTest.java +++ b/src/test/java/org/wise/portal/presentation/web/controllers/teacher/management/RemoveStudentRunControllerTest.java @@ -59,7 +59,7 @@ public void removeStudent_TargetUserIsTeacher_ThrowAccessDenied() throws Excepti } @Test - public void removeStudent_Student_RemoveStudentFromRun() throws Exception { + public void removeStudent_TargetUserIsStudent_RemoveStudentFromRun() throws Exception { expect(runService.retrieveById(runId1)).andReturn(run1); expect(runService.hasWritePermission(teacherAuth, run1)).andReturn(true); expect(userService.retrieveById(student1Id)).andReturn(student1); From df2f0ba00fea6df84e47e5d703543c5d6b23f0cf Mon Sep 17 00:00:00 2001 From: Isaries Date: Fri, 31 Jul 2026 21:01:08 +0800 Subject: [PATCH 3/3] fix(security): say the removal is scoped to the run in the denial message The message thrown when the target is not a student did not mention the run, unlike the one thrown when write permission on the run is missing. --- .../teacher/management/RemoveStudentRunController.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/wise/portal/presentation/web/controllers/teacher/management/RemoveStudentRunController.java b/src/main/java/org/wise/portal/presentation/web/controllers/teacher/management/RemoveStudentRunController.java index 751508462..c37ccf8a1 100644 --- a/src/main/java/org/wise/portal/presentation/web/controllers/teacher/management/RemoveStudentRunController.java +++ b/src/main/java/org/wise/portal/presentation/web/controllers/teacher/management/RemoveStudentRunController.java @@ -34,7 +34,8 @@ public void removeStudent(Authentication auth, @PathVariable Long runId, if (runService.hasWritePermission(auth, run)) { User studentUser = userService.retrieveById(studentId); if (!studentUser.isStudent()) { - throw new AccessDeniedException("User does not have permission to remove this user"); + throw new AccessDeniedException( + "User does not have permission to remove this user from run"); } studentService.removeStudentFromRun(studentUser, run); } else {