From 3b5cafbedaa646603458227b0f49543dd983f157 Mon Sep 17 00:00:00 2001 From: Isaries Date: Thu, 16 Jul 2026 16:33:41 +0800 Subject: [PATCH 1/2] fix(security): verify a student belongs to the run before changing their password The change student password endpoint checked that the signed-in teacher has write permission on the run taken from the path, but then loaded the target user directly from the student id without checking that the two are related. The id was neither confined to the run nor to students. Verify that the target is a student of that run before changing the password. --- .../ChangeStudentPasswordController.java | 4 +++ .../ChangeStudentPasswordControllerTest.java | 30 +++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/src/main/java/org/wise/portal/presentation/web/controllers/teacher/management/ChangeStudentPasswordController.java b/src/main/java/org/wise/portal/presentation/web/controllers/teacher/management/ChangeStudentPasswordController.java index b724c27fe..50112cc9f 100644 --- a/src/main/java/org/wise/portal/presentation/web/controllers/teacher/management/ChangeStudentPasswordController.java +++ b/src/main/java/org/wise/portal/presentation/web/controllers/teacher/management/ChangeStudentPasswordController.java @@ -47,6 +47,10 @@ ResponseEntity> changeStudentPassword(Authentication auth, return ResponseEntityGenerator.createError(passwordService.getErrors(newStudentPassword)); } User studentUser = userService.retrieveById(studentId); + if (!run.isStudentAssociatedToThisRun(studentUser)) { + throw new AccessDeniedException( + "User does not have permission to change this student's password"); + } userService.updateUserPassword(studentUser, newStudentPassword); return ResponseEntityGenerator.createSuccess("passwordUpdated"); } else { diff --git a/src/test/java/org/wise/portal/presentation/web/controllers/teacher/management/ChangeStudentPasswordControllerTest.java b/src/test/java/org/wise/portal/presentation/web/controllers/teacher/management/ChangeStudentPasswordControllerTest.java index c686852e7..4de7af64e 100644 --- a/src/test/java/org/wise/portal/presentation/web/controllers/teacher/management/ChangeStudentPasswordControllerTest.java +++ b/src/test/java/org/wise/portal/presentation/web/controllers/teacher/management/ChangeStudentPasswordControllerTest.java @@ -115,6 +115,36 @@ public void changeStudentPassword_InvalidPassword_ReturnError() throws Exception verifyServices(); } + @Test + public void changeStudentPassword_StudentNotInRun_ThrowAccessDenied() throws Exception { + setupChangeStudentPasswordExpect(); + expect(userService.isPasswordCorrect(teacher1, TEACHER_PASSWORD_CORRECT)).andReturn(true); + expect(userService.retrieveById(student2Id)).andReturn(student2); + replayServices(); + try { + controller.changeStudentPassword(teacherAuth, runId1, student2Id, TEACHER_PASSWORD_CORRECT, + STUDENT_PASSWORD_VALID); + fail("Expected AccessDeniedException to be thrown"); + } catch (AccessDeniedException e) { + } + verifyServices(); + } + + @Test + public void changeStudentPassword_TargetUserIsTeacher_ThrowAccessDenied() throws Exception { + setupChangeStudentPasswordExpect(); + expect(userService.isPasswordCorrect(teacher1, TEACHER_PASSWORD_CORRECT)).andReturn(true); + expect(userService.retrieveById(teacher2Id)).andReturn(teacher2); + replayServices(); + try { + controller.changeStudentPassword(teacherAuth, runId1, teacher2Id, TEACHER_PASSWORD_CORRECT, + STUDENT_PASSWORD_VALID); + fail("Expected AccessDeniedException to be thrown"); + } catch (AccessDeniedException e) { + } + verifyServices(); + } + @Test public void changeStudentPassword_validTeacherPassword_ChangeStudentPassword() throws Exception { setupChangeStudentPasswordExpect(); From 88d5223b45ce80a2d64cda0ec94b5379d0973811 Mon Sep 17 00:00:00 2001 From: Isaries Date: Sat, 18 Jul 2026 20:01:15 +0800 Subject: [PATCH 2/2] refactor(security): consolidate authorization check into isAllowed Follow the CreateWorkgroupController pattern: check run write permission and student-run association in one short-circuited predicate, instead of a separate check nested inside the password flow. Keeps the student lookup behind the permission gate so an unauthorized caller never triggers it. Co-authored-by: Aaron Detre <168240861+Aaron-Detre@users.noreply.github.com> --- .../management/ChangeStudentPasswordController.java | 12 +++++++----- .../ChangeStudentPasswordControllerTest.java | 9 +++++---- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/wise/portal/presentation/web/controllers/teacher/management/ChangeStudentPasswordController.java b/src/main/java/org/wise/portal/presentation/web/controllers/teacher/management/ChangeStudentPasswordController.java index 50112cc9f..dce190769 100644 --- a/src/main/java/org/wise/portal/presentation/web/controllers/teacher/management/ChangeStudentPasswordController.java +++ b/src/main/java/org/wise/portal/presentation/web/controllers/teacher/management/ChangeStudentPasswordController.java @@ -39,7 +39,7 @@ ResponseEntity> changeStudentPassword(Authentication auth, @RequestParam String newStudentPassword) throws ObjectNotFoundException, IncorrectPasswordException { Run run = runService.retrieveById(runId); - if (runService.hasWritePermission(auth, run)) { + if (isAllowed(auth, run, studentId)) { User teacherUser = userService.retrieveUserByUsername(auth.getName()); boolean isTeacherGoogleUser = teacherUser.getUserDetails().isGoogleUser(); if (isTeacherPasswordCorrect(isTeacherGoogleUser, teacherUser, teacherPassword)) { @@ -47,10 +47,6 @@ ResponseEntity> changeStudentPassword(Authentication auth, return ResponseEntityGenerator.createError(passwordService.getErrors(newStudentPassword)); } User studentUser = userService.retrieveById(studentId); - if (!run.isStudentAssociatedToThisRun(studentUser)) { - throw new AccessDeniedException( - "User does not have permission to change this student's password"); - } userService.updateUserPassword(studentUser, newStudentPassword); return ResponseEntityGenerator.createSuccess("passwordUpdated"); } else { @@ -62,6 +58,12 @@ ResponseEntity> changeStudentPassword(Authentication auth, } } + private boolean isAllowed(Authentication auth, Run run, Long studentId) + throws ObjectNotFoundException { + return runService.hasWritePermission(auth, run) + && run.isStudentAssociatedToThisRun(userService.retrieveById(studentId)); + } + private boolean isTeacherPasswordCorrect(boolean isGoogleUser, User teacherUser, String password) { return isGoogleUser || userService.isPasswordCorrect(teacherUser, password); diff --git a/src/test/java/org/wise/portal/presentation/web/controllers/teacher/management/ChangeStudentPasswordControllerTest.java b/src/test/java/org/wise/portal/presentation/web/controllers/teacher/management/ChangeStudentPasswordControllerTest.java index 4de7af64e..426c7d216 100644 --- a/src/test/java/org/wise/portal/presentation/web/controllers/teacher/management/ChangeStudentPasswordControllerTest.java +++ b/src/test/java/org/wise/portal/presentation/web/controllers/teacher/management/ChangeStudentPasswordControllerTest.java @@ -75,6 +75,7 @@ public void changeStudentPassword_NoWritePermission_ThrowAccessDenied() throws E private void setupChangeStudentPasswordExpect() throws Exception { expect(runService.retrieveById(runId1)).andReturn(run1); expect(runService.hasWritePermission(teacherAuth, run1)).andReturn(true); + expect(userService.retrieveById(student1Id)).andReturn(student1); expect(userService.retrieveUserByUsername(TEACHER_USERNAME)).andReturn(teacher1); } @@ -117,8 +118,8 @@ public void changeStudentPassword_InvalidPassword_ReturnError() throws Exception @Test public void changeStudentPassword_StudentNotInRun_ThrowAccessDenied() throws Exception { - setupChangeStudentPasswordExpect(); - expect(userService.isPasswordCorrect(teacher1, TEACHER_PASSWORD_CORRECT)).andReturn(true); + expect(runService.retrieveById(runId1)).andReturn(run1); + expect(runService.hasWritePermission(teacherAuth, run1)).andReturn(true); expect(userService.retrieveById(student2Id)).andReturn(student2); replayServices(); try { @@ -132,8 +133,8 @@ public void changeStudentPassword_StudentNotInRun_ThrowAccessDenied() throws Exc @Test public void changeStudentPassword_TargetUserIsTeacher_ThrowAccessDenied() throws Exception { - setupChangeStudentPasswordExpect(); - expect(userService.isPasswordCorrect(teacher1, TEACHER_PASSWORD_CORRECT)).andReturn(true); + expect(runService.retrieveById(runId1)).andReturn(run1); + expect(runService.hasWritePermission(teacherAuth, run1)).andReturn(true); expect(userService.retrieveById(teacher2Id)).andReturn(teacher2); replayServices(); try {