From 5c0e6852440b363df2ded7258a1992e0dc1e7efc Mon Sep 17 00:00:00 2001 From: David Hay Date: Thu, 30 Apr 2026 09:00:24 +0100 Subject: [PATCH 1/7] CICD-TMP-FILE-CLEANUP - reduce the amount of logging by TempFileCleanerExtension and TempFileCleaner. --- .../common/util/LogLevelExtension.java | 70 +++++++++++++++++++ .../common/util/TempFileCleaner.java | 18 ++--- .../common/util/TempFileCleanerExtension.java | 4 +- .../util/TempFileCleanerExtensionTest.java | 19 ++++- 4 files changed, 95 insertions(+), 16 deletions(-) create mode 100644 datavault-common/src/test/java/org/datavaultplatform/common/util/LogLevelExtension.java diff --git a/datavault-common/src/test/java/org/datavaultplatform/common/util/LogLevelExtension.java b/datavault-common/src/test/java/org/datavaultplatform/common/util/LogLevelExtension.java new file mode 100644 index 000000000..422daece6 --- /dev/null +++ b/datavault-common/src/test/java/org/datavaultplatform/common/util/LogLevelExtension.java @@ -0,0 +1,70 @@ +package org.datavaultplatform.common.util; + +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.LoggerContext; +import org.junit.jupiter.api.extension.BeforeAllCallback; +import org.junit.jupiter.api.extension.ExtensionContext; +import org.slf4j.LoggerFactory; + +import java.util.HashMap; +import java.util.Map; + +/** + * A JUnit 5 extension to temporarily change the logging level of specific loggers for a test class. + * It restores the original logging levels after all tests in the class have run. + */ +public class LogLevelExtension implements BeforeAllCallback, ExtensionContext.Store.CloseableResource { + + private static final String LOG_LEVEL_STORE_KEY = "logLevels"; + private final Map loggersToChange; // Loggers whose levels we want to change + private Map originalLevels; // Store original levels here + + /** + * Constructor to specify which loggers to modify and their desired temporary levels. + * + * @param loggerName The name of the logger to modify (e.g., "org.datavaultplatform.worker"). + * @param newLevel The temporary logging level (e.g., Level.OFF, Level.DEBUG). + */ + public LogLevelExtension(String loggerName, Level newLevel) { + this.loggersToChange = new HashMap<>(); + this.loggersToChange.put(loggerName, newLevel); + } + + /** + * Constructor to specify multiple loggers to modify. + * + * @param loggersToChange A map where keys are logger names and values are their desired temporary levels. + */ + public LogLevelExtension(Map loggersToChange) { + this.loggersToChange = new HashMap<>(loggersToChange); + } + + @Override + public void beforeAll(ExtensionContext context) { + // Initialize the map to store original levels for this instance + this.originalLevels = new HashMap<>(); + + LoggerContext loggerContext = (LoggerContext) LoggerFactory.getILoggerFactory(); + + for (Map.Entry entry : loggersToChange.entrySet()) { + Logger logger = loggerContext.getLogger(entry.getKey()); + this.originalLevels.put(entry.getKey(), logger.getLevel()); // Store original level in the instance field + logger.setLevel(entry.getValue()); // Set new level + } + // Store 'this' instance in the global store, so its close() method is called later by JUnit. + // The unique ID of the context is a good key to ensure uniqueness per test class. + context.getStore(ExtensionContext.Namespace.GLOBAL).put(context.getUniqueId(), this); + } + + @Override + public void close() { + LoggerContext loggerContext = (LoggerContext) LoggerFactory.getILoggerFactory(); + + // Access originalLevels directly from the instance field + for (Map.Entry entry : this.originalLevels.entrySet()) { + Logger logger = loggerContext.getLogger(entry.getKey()); + logger.setLevel(entry.getValue()); // Restore original level + } + } +} \ No newline at end of file diff --git a/datavault-common/src/test/java/org/datavaultplatform/common/util/TempFileCleaner.java b/datavault-common/src/test/java/org/datavaultplatform/common/util/TempFileCleaner.java index 2943ce53e..63cdee85d 100644 --- a/datavault-common/src/test/java/org/datavaultplatform/common/util/TempFileCleaner.java +++ b/datavault-common/src/test/java/org/datavaultplatform/common/util/TempFileCleaner.java @@ -57,12 +57,9 @@ public static void cleanTempDirectories(File baseTemp, Instant startTime) { } try { FileUtils.deleteDirectory(dir); - log.info("Deleted temp test directory: {}", dir.getAbsolutePath()); + log.debug("Deleted temp test directory: {}", dir.getAbsolutePath()); } catch (Exception ex) { log.warn("Failed to delete directory: " + dir.getAbsolutePath(), ex); - } finally { - log.info("-"); - } } } @@ -76,12 +73,9 @@ public static void cleanTempFiles(File baseTemp, Instant startTime) { } try { var deleted = Files.deleteIfExists(file.toPath()); - log.info("Deleted temp test file?: {} / {}", file.getAbsolutePath(), deleted); + log.debug("Deleted temp test file?: {} / {}", file.getAbsolutePath(), deleted); } catch (Exception ex) { - log.warn("Failed to delete file: " + file.getAbsolutePath(), ex); - } finally { - log.info("-"); - + log.warn("Failed to delete file: {}", file.getAbsolutePath(), ex); } } } @@ -101,7 +95,7 @@ public static List findDirectories(File baseTemp, Instant startTime) { } dirsToDelete.forEach(path -> { - log.info("XX Deleting directory: {}", path); + log.debug("XX Deleting directory: {}", path); }); return dirsToDelete; } @@ -121,7 +115,7 @@ public static List findFiles(File baseTemp, Instant startTime) { } filesToDelete.forEach(path -> { - log.info("XX Deleting file: {}", path); + log.debug("XX Deleting file: {}", path); }); return filesToDelete; @@ -156,7 +150,7 @@ private static boolean isEligibleForDeletion(Path path, Instant testStartTime) { // touched during the test rather than freshly created) boolean result = !created.isBefore(threshold) || !modified.isBefore(threshold); if(result) { - log.info("XX {} isEligibleForDeletion? {}", path, result); + log.debug("XX {} isEligibleForDeletion? {}", path, result); } return result; } catch (IOException e) { diff --git a/datavault-common/src/test/java/org/datavaultplatform/common/util/TempFileCleanerExtension.java b/datavault-common/src/test/java/org/datavaultplatform/common/util/TempFileCleanerExtension.java index 47d34e39b..695173163 100644 --- a/datavault-common/src/test/java/org/datavaultplatform/common/util/TempFileCleanerExtension.java +++ b/datavault-common/src/test/java/org/datavaultplatform/common/util/TempFileCleanerExtension.java @@ -31,11 +31,9 @@ public void afterAll(ExtensionContext context) { Instant startTime = context.getStore(ExtensionContext.Namespace.create(getClass(), context.getRequiredTestClass())) .get("testStartTime", Instant.class); if (startTime != null) { - log.info("XXXX-XXXX-XXXX-XXXX-XXXX-XXXX-XXXX-XXXX"); TempFileCleaner.cleanTempTestFiles(startTime); } else { - // Fallback if beforeAll wasn't called for some reason (e.g., test failed before beforeAll completed) - TempFileCleaner.cleanTempTestFiles(Instant.EPOCH); + log.error("No test start time found for test class {}", context.getRequiredTestClass().getName()); } } } diff --git a/datavault-common/src/test/java/org/datavaultplatform/common/util/TempFileCleanerExtensionTest.java b/datavault-common/src/test/java/org/datavaultplatform/common/util/TempFileCleanerExtensionTest.java index 971cdcaa0..d3c9b84f0 100644 --- a/datavault-common/src/test/java/org/datavaultplatform/common/util/TempFileCleanerExtensionTest.java +++ b/datavault-common/src/test/java/org/datavaultplatform/common/util/TempFileCleanerExtensionTest.java @@ -1,17 +1,33 @@ package org.datavaultplatform.common.util; +import ch.qos.logback.classic.Level; import lombok.SneakyThrows; import lombok.extern.slf4j.Slf4j; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; -import java.nio.file.*; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Map; @Slf4j +@ExtendWith({TempFileCleanerExtensionTest.MyLogLevelExtension.class, TempFileCleanerExtension.class}) class TempFileCleanerExtensionTest { + + // Define the logger levels you want to change for this test class + static class MyLogLevelExtension extends LogLevelExtension { + MyLogLevelExtension() { + super(Map.of( + "org.datavaultplatform.common.util", Level.DEBUG + )); + } + } @Test @SneakyThrows void testJavaTmpDirDelete() { + log.debug("testJavaTmpDirDelete"); Path tempDir = Paths.get(System.getProperty("java.io.tmpdir")); Files.writeString(tempDir.resolve("testNonSlash102.txt"), "Hello world!\n"); @@ -23,6 +39,7 @@ void testJavaTmpDirDelete() { @Test @SneakyThrows void testSlashTmpDirDelete() { + log.debug("testSlashTmpDirDelete"); Path path = Paths.get("/tmp/deleteSlash/test100.txt"); // Create parent directories if they don't exist From 29f320bb09b1503611b1f6dbaa2176c8c3caf3cc Mon Sep 17 00:00:00 2001 From: David Hay Date: Thu, 30 Apr 2026 14:46:49 +0100 Subject: [PATCH 2/7] LUC070-252-fix-urgent-pentest-findings - fix authentication problems around /admin/users/* --- .../admin/AdminUsersController.java | 11 +- ...dminUsersControllerNonShibProfileTest.java | 64 +++++ .../AdminUsersControllerShibProfileTest.java | 62 +++++ ...dminUsersControllerNonShibProfileTest.java | 233 ++++++++++++++++++ 4 files changed, 368 insertions(+), 2 deletions(-) create mode 100644 datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersControllerNonShibProfileTest.java create mode 100644 datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersControllerShibProfileTest.java create mode 100644 datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/BaseAdminUsersControllerNonShibProfileTest.java diff --git a/datavault-webapp/src/main/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersController.java b/datavault-webapp/src/main/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersController.java index 0ed7c802d..b7e60c813 100644 --- a/datavault-webapp/src/main/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersController.java +++ b/datavault-webapp/src/main/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersController.java @@ -5,6 +5,8 @@ import org.datavaultplatform.webapp.services.RestService; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; +import org.springframework.http.MediaType; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.stereotype.Controller; import org.springframework.ui.ModelMap; import org.springframework.web.bind.annotation.*; @@ -25,6 +27,7 @@ public AdminUsersController(RestService restService) { this.restService = restService; } + @PreAuthorize("hasRole('IS_ADMIN')") @RequestMapping(value = "/admin/users", method = RequestMethod.GET) public String getUsersListing(ModelMap model, @RequestParam(value = "query", required = false) String query) throws Exception { @@ -40,7 +43,8 @@ public String getUsersListing(ModelMap model, } // Return an empty 'create new user' page - @RequestMapping(value = "/admin/users/create", method = RequestMethod.GET) + @PreAuthorize("hasRole('IS_ADMIN') and !@environment.acceptsProfiles('shib')") + @GetMapping(value = "/admin/users/create", produces = MediaType.TEXT_HTML_VALUE) public String createUser(ModelMap model) throws Exception { // pass the view an empty User since the form expects it model.addAttribute("user", new User()); @@ -49,7 +53,8 @@ public String createUser(ModelMap model) throws Exception { } // Process the completed 'create new user' page - @RequestMapping(value = "/admin/users/create", method = RequestMethod.POST) + @PreAuthorize("hasRole('IS_ADMIN') and !@environment.acceptsProfiles('shib')") + @PostMapping(value = "/admin/users/create", produces = MediaType.TEXT_HTML_VALUE) public String addUser(@ModelAttribute User user, ModelMap model, @RequestParam String action) throws Exception { // Was the cancel button pressed? if ("cancel".equals(action)) { @@ -68,6 +73,7 @@ public String addUser(@ModelAttribute User user, ModelMap model, @RequestParam S } // Return an 'edit user' page + @PreAuthorize("hasRole('IS_ADMIN') or #userID == authentication.name") @RequestMapping(value = "/admin/users/edit/{userid}", method = RequestMethod.GET) public String editUser(ModelMap model, @PathVariable("userid") String userID) throws Exception { @@ -76,6 +82,7 @@ public String editUser(ModelMap model, @PathVariable("userid") String userID) th } // Process the completed 'edit user' page + @PreAuthorize("hasRole('IS_ADMIN') or #userID == authentication.name") @RequestMapping(value = "/admin/users/edit/{userid}", method = RequestMethod.POST) public String editUser(@ModelAttribute User user, ModelMap model, @PathVariable("userid") String userID, @RequestParam String action) throws Exception { // Was the cancel button pressed? diff --git a/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersControllerNonShibProfileTest.java b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersControllerNonShibProfileTest.java new file mode 100644 index 000000000..ab58afd10 --- /dev/null +++ b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersControllerNonShibProfileTest.java @@ -0,0 +1,64 @@ +package org.datavaultplatform.webapp.controllers.admin; + +import lombok.SneakyThrows; +import org.datavaultplatform.webapp.app.DataVaultWebApp; +import org.datavaultplatform.webapp.test.AddTestProperties; +import org.datavaultplatform.webapp.test.ProfileDatabase; + +import org.junit.jupiter.api.*; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.http.MediaType; +import org.springframework.security.test.context.support.WithMockUser; +import org.springframework.test.context.TestPropertySource; + +import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; + +@SpringBootTest(classes = DataVaultWebApp.class, webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) +@ProfileDatabase +@TestPropertySource(properties = "logging.level.org.springframework.security=DEBUG") +@AddTestProperties +@AutoConfigureMockMvc +@TestMethodOrder(MethodOrderer.OrderAnnotation.class) +class AdminUsersControllerNonShibProfileTest extends baseAdminUsersControllerTest { + + @Override + String getExpectedProfile() { + return "database"; + } + + @Test + @Order(5) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"IS_ADMIN", "USER"}) + void testShowAddUserForm_SuperUserShowCreateUserForm() { + mockMvc.perform(get("/admin/users/create")).andDo(print()) + .andExpect(status().isOk()) + .andReturn(); + } + + @Test + @Order(7) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"IS_ADMIN", "USER"}) + void testSubmitAddUserForm_SuperUserSubmitCreateUserForm() { + mockMvc.perform(post("/admin/users/create") + .contentType(MediaType.APPLICATION_FORM_URLENCODED) + .with(csrf()) + .param("action", "Save") + .param("firstname", "Robert") + .param("lastname", "Roberts") + .param("email", "bob.123@example.com") + .param("id", "bob")) + .andDo(print()) + .andExpect(status().isFound()) + .andExpect(redirectedUrl("/admin/users")) + .andReturn(); + } + + +} \ No newline at end of file diff --git a/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersControllerShibProfileTest.java b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersControllerShibProfileTest.java new file mode 100644 index 000000000..44648be5b --- /dev/null +++ b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersControllerShibProfileTest.java @@ -0,0 +1,62 @@ +package org.datavaultplatform.webapp.controllers.admin; + +import lombok.SneakyThrows; +import org.datavaultplatform.webapp.app.DataVaultWebApp; +import org.datavaultplatform.webapp.test.AddTestProperties; +import org.datavaultplatform.webapp.test.ProfileShib; + +import org.junit.jupiter.api.*; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.http.MediaType; +import org.springframework.security.test.context.support.WithMockUser; +import org.springframework.test.context.TestPropertySource; + +import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; + +@SpringBootTest(classes = DataVaultWebApp.class, webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) +@ProfileShib +@TestPropertySource(properties = "logging.level.org.springframework.security=DEBUG") +@AddTestProperties +@AutoConfigureMockMvc +class AdminUsersControllerShibProfileTest extends baseAdminUsersControllerTest { + + @Override + String getExpectedProfile() { + return "shib"; + } + + @Test + @Order(5) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"IS_ADMIN", "USER"}) + void testShowAddUserForm_SuperUserShowCreateUserForm() { + mockMvc.perform(get("/admin/users/create")).andDo(print()) + .andExpect(status().isForbidden()) + .andReturn(); + } + + @Test + @Order(7) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"IS_ADMIN", "USER"}) + void testSubmitAddUserForm_SuperUserSubmitCreateUserForm() { + mockMvc.perform(post("/admin/users/create") + .contentType(MediaType.APPLICATION_FORM_URLENCODED) + .with(csrf()) + .param("action", "Save") + .param("firstname", "Robert") + .param("lastname", "Roberts") + .param("email", "bob.123@example.com") + .param("id", "bob")) + .andDo(print()) + .andExpect(status().isForbidden()) + .andReturn(); + } + + +} \ No newline at end of file diff --git a/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/BaseAdminUsersControllerNonShibProfileTest.java b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/BaseAdminUsersControllerNonShibProfileTest.java new file mode 100644 index 000000000..c66308f64 --- /dev/null +++ b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/BaseAdminUsersControllerNonShibProfileTest.java @@ -0,0 +1,233 @@ +package org.datavaultplatform.webapp.controllers.admin; + +import lombok.SneakyThrows; +import org.datavaultplatform.common.model.User; +import org.datavaultplatform.webapp.services.RestService; +import org.hamcrest.Matchers; +import org.junit.jupiter.api.*; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.core.env.Environment; +import org.springframework.http.MediaType; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.test.context.support.WithMockUser; +import org.springframework.test.context.bean.override.mockito.MockitoBean; +import org.springframework.test.web.servlet.MockMvc; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.*; +import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; + +@SuppressWarnings("DefaultAnnotationParam") +abstract class baseAdminUsersControllerTest { + + @Autowired + Environment env; + + @MockitoBean + RestService restService; + + @Autowired + AdminUsersController adminUsersController; + + @Autowired + MockMvc mockMvc; + + @BeforeEach + final void setup() { + assertThat(adminUsersController).isNotNull(); + assertThat(env.getActiveProfiles()).containsExactly(getExpectedProfile()); + } + + abstract String getExpectedProfile(); + + @Test + @Order(2) + @SneakyThrows + @WithMockUser(username = "vanilla-user") + void testListUsers_VanillaUserCannotListUsers() { + mockMvc.perform(get("/admin/users").accept(MediaType.TEXT_HTML)).andDo(print()) + .andExpect(status().isForbidden()) + .andReturn(); + } + + @Test + @Order(3) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"IS_ADMIN", "USER"}) + void testListUsers_SuperUserCanListUsers() { + mockMvc.perform(get("/admin/users").accept(MediaType.TEXT_HTML)).andDo(print()) + .andExpect(status().isOk()) + .andExpect(content().contentTypeCompatibleWith(MediaType.TEXT_HTML)) + .andExpect(content().string(Matchers.containsString("Admin - Users"))) + .andReturn(); + } + + + @Test + @Order(4) + @SneakyThrows + @WithMockUser(username = "vanilla-user", roles = {"USER"}) + void testShowAddUserForm_VanillaUserCannotShowCreateUserForm() { + mockMvc.perform(get("/admin/users/create")).andDo(print()) + .andExpect(status().isForbidden()) + .andReturn(); + } + + abstract void testShowAddUserForm_SuperUserShowCreateUserForm(); + + @Test + @Order(6) + @SneakyThrows + @WithMockUser(username = "vanilla-user", roles = {"USER"}) + void testSubmitAddUserForm_VanillaUserCannotSubmitCreateUserForm() { + mockMvc.perform(post("/admin/users/create") + .contentType(MediaType.APPLICATION_FORM_URLENCODED) + .with(csrf()) + .param("action", "Save") + .param("firstname", "Robert") + .param("lastname", "Roberts") + .param("email", "bob.123@example.com") + .param("id", "bob")).andDo(print()) + .andExpect(status().isForbidden()) + .andReturn(); + } + + abstract void testSubmitAddUserForm_SuperUserSubmitCreateUserForm(); + + @Test + @Order(8) + @SneakyThrows + @WithMockUser(username = "vanilla-user", roles = {"USER"}) + void testShowEditUsersForm_UserCannotShowEditUserFormForThemselves() { + mockMvc.perform(get("/admin/users/edit/bob")).andDo(print()) + .andExpect(status().isForbidden()) + .andReturn(); + + verifyNoMoreInteractions(restService); + } + + @Test + @Order(9) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"IS_ADMIN", "USER"}) + void testShowEditUsersForm_SuperUserCanShowCreateUserForm() { + User bobUser = new User(); + bobUser.setID("bob"); + bobUser.setFirstname("Robert"); + bobUser.setLastname("Roberts"); + bobUser.setEmail("bob.123@example.com"); + when(restService.getUser("bob")).thenReturn(bobUser); + mockMvc.perform(get("/admin/users/edit/bob")).andDo(print()) + .andExpect(status().isOk()) + .andReturn(); + + verify(restService).getUser("bob"); + } + + @Test + @Order(10) + @SneakyThrows + @WithMockUser(username = "bob", roles = {"USER"}) + void testShowEditUsersForm_UserCanShowEditUserFormForThemselves() { + + Authentication auth = SecurityContextHolder.getContext().getAuthentication(); + assertThat(auth.getName()).isEqualTo("bob"); + User bobUser = new User(); + bobUser.setID("bob"); + bobUser.setFirstname("Robert"); + bobUser.setLastname("Roberts"); + bobUser.setEmail("bob.123@example.com"); + when(restService.getUser("bob")).thenReturn(bobUser); + mockMvc.perform(get("/admin/users/edit/bob")).andDo(print()) + .andExpect(status().isOk()) + .andReturn(); + + verify(restService).getUser("bob"); + } + + @Test + @Order(11) + @SneakyThrows + @WithMockUser(username = "vanilla-user", roles = {"USER"}) + void testSubmitEditUsersForm_UserCannotSubmitEditUserFormForOthers() { + mockMvc.perform(post("/admin/users/edit/bob") + .contentType(MediaType.APPLICATION_FORM_URLENCODED) + .with(csrf()) + .param("action", "Save") + .param("firstname", "Robert") + .param("lastname", "Roberts") + .param("email", "bob.123@example.com") + .param("id", "bob") + ).andDo(print()) + .andExpect(status().isForbidden()) + .andReturn(); + + verifyNoMoreInteractions(restService); + } + + @Test + @Order(12) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"IS_ADMIN", "USER"}) + void testSubmitEditUsersForm_SuperAdminCanSubmitEditUserFormForOthers() { + + User bobUser = new User(); + bobUser.setID("bob"); + bobUser.setFirstname("Robert"); + bobUser.setLastname("Roberts"); + bobUser.setEmail("bob.123@example.com"); + + when(restService.getUser("bob")).thenReturn(bobUser); + + mockMvc.perform(post("/admin/users/edit/bob") + .contentType(MediaType.APPLICATION_FORM_URLENCODED) + .with(csrf()) + .param("action", "Save") + .param("firstname", "Robert1") + .param("lastname", "Roberts1") + .param("email", "bob.1234@example.com") + .param("id", "bob")) + .andDo(print()) + .andExpect(status().isOk()) + .andReturn(); + + verify(restService).getUser("bob"); + verify(restService).editUser(bobUser); + verifyNoMoreInteractions(restService); + } + + @Test + @Order(13) + @SneakyThrows + @WithMockUser(username = "bob", roles = {"USER"}) + void testSubmitEditUsersForm_UserCanSubmitEditUserFormForThemselves() { + + User bobUser = new User(); + bobUser.setID("bob"); + bobUser.setFirstname("Robert"); + bobUser.setLastname("Roberts"); + bobUser.setEmail("bob.123@example.com"); + when(restService.getUser("bob")).thenReturn(bobUser); + + mockMvc.perform(post("/admin/users/edit/bob") + .contentType(MediaType.APPLICATION_FORM_URLENCODED) + .with(csrf()) + .param("action", "Save") + .param("firstname", "Robert1") + .param("lastname", "Roberts1") + .param("email", "bob.1234@example.com") + .param("id", "bob")) + .andDo(print()) + .andExpect(status().isOk()) + .andReturn(); + + verify(restService).getUser("bob"); + verify(restService).editUser(bobUser); + verifyNoMoreInteractions(restService); + } +} \ No newline at end of file From 7eb3d3b2b0b61096ad101f8e5089c31674c52291 Mon Sep 17 00:00:00 2001 From: David Hay Date: Thu, 30 Apr 2026 15:14:54 +0100 Subject: [PATCH 3/7] LUC070-252-fix-urgent-pentest-findings - renamed files to match classes. --- .../admin/AdminUsersControllerNonShibProfileTest.java | 2 +- .../controllers/admin/AdminUsersControllerShibProfileTest.java | 2 +- ...onShibProfileTest.java => BaseAdminUsersControllerTest.java} | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) rename datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/{BaseAdminUsersControllerNonShibProfileTest.java => BaseAdminUsersControllerTest.java} (99%) diff --git a/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersControllerNonShibProfileTest.java b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersControllerNonShibProfileTest.java index ab58afd10..104b7666f 100644 --- a/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersControllerNonShibProfileTest.java +++ b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersControllerNonShibProfileTest.java @@ -24,7 +24,7 @@ @AddTestProperties @AutoConfigureMockMvc @TestMethodOrder(MethodOrderer.OrderAnnotation.class) -class AdminUsersControllerNonShibProfileTest extends baseAdminUsersControllerTest { +class AdminUsersControllerNonShibProfileTest extends BaseAdminUsersControllerTest { @Override String getExpectedProfile() { diff --git a/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersControllerShibProfileTest.java b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersControllerShibProfileTest.java index 44648be5b..6820002d7 100644 --- a/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersControllerShibProfileTest.java +++ b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminUsersControllerShibProfileTest.java @@ -23,7 +23,7 @@ @TestPropertySource(properties = "logging.level.org.springframework.security=DEBUG") @AddTestProperties @AutoConfigureMockMvc -class AdminUsersControllerShibProfileTest extends baseAdminUsersControllerTest { +class AdminUsersControllerShibProfileTest extends BaseAdminUsersControllerTest { @Override String getExpectedProfile() { diff --git a/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/BaseAdminUsersControllerNonShibProfileTest.java b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/BaseAdminUsersControllerTest.java similarity index 99% rename from datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/BaseAdminUsersControllerNonShibProfileTest.java rename to datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/BaseAdminUsersControllerTest.java index c66308f64..6786c5e6b 100644 --- a/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/BaseAdminUsersControllerNonShibProfileTest.java +++ b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/BaseAdminUsersControllerTest.java @@ -23,7 +23,7 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; @SuppressWarnings("DefaultAnnotationParam") -abstract class baseAdminUsersControllerTest { +abstract class BaseAdminUsersControllerTest { @Autowired Environment env; From c8f79857ad2846e8a219f11491bdc84fe0477a1d Mon Sep 17 00:00:00 2001 From: David Hay Date: Thu, 30 Apr 2026 16:33:56 +0100 Subject: [PATCH 4/7] LUC070-252-fix-urgent-pentest-findings - added security controls to /admin/pendingVaults/** for IS_ADMIN role --- .../webapp/config/HttpSecurityUtils.java | 1 + .../AdminPendingVaultsControllerTest.java | 81 +++++++++++++++++++ 2 files changed, 82 insertions(+) create mode 100644 datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminPendingVaultsControllerTest.java diff --git a/datavault-webapp/src/main/java/org/datavaultplatform/webapp/config/HttpSecurityUtils.java b/datavault-webapp/src/main/java/org/datavaultplatform/webapp/config/HttpSecurityUtils.java index 99332a35f..375b97121 100644 --- a/datavault-webapp/src/main/java/org/datavaultplatform/webapp/config/HttpSecurityUtils.java +++ b/datavault-webapp/src/main/java/org/datavaultplatform/webapp/config/HttpSecurityUtils.java @@ -33,6 +33,7 @@ public static void authorizeRequests( authz.requestMatchers("/admin/paused/retrieve/history").hasRole("USER"); authz.requestMatchers("/admin/paused/retrieve/toggle").hasRole("IS_ADMIN"); + authz.requestMatchers("/admin/pendingVaults/**").hasRole("IS_ADMIN"); authz.requestMatchers("/admin/archivestores/**").hasAuthority("ROLE_ADMIN_ARCHIVESTORES"); authz.requestMatchers("/admin/billing/**").hasAuthority("ROLE_ADMIN_BILLING"); diff --git a/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminPendingVaultsControllerTest.java b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminPendingVaultsControllerTest.java new file mode 100644 index 000000000..92b739dd1 --- /dev/null +++ b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminPendingVaultsControllerTest.java @@ -0,0 +1,81 @@ +package org.datavaultplatform.webapp.controllers.admin; + +import lombok.SneakyThrows; +import org.datavaultplatform.webapp.app.DataVaultWebApp; +import org.datavaultplatform.webapp.services.RestService; +import org.datavaultplatform.webapp.test.AddTestProperties; +import org.datavaultplatform.webapp.test.ProfileDatabase; +import org.junit.jupiter.api.*; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.core.env.Environment; +import org.springframework.security.test.context.support.WithMockUser; +import org.springframework.test.context.TestPropertySource; +import org.springframework.test.context.bean.override.mockito.MockitoBean; +import org.springframework.test.web.servlet.MockMvc; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +@SuppressWarnings("DefaultAnnotationParam") +@SpringBootTest(classes = DataVaultWebApp.class, webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) +@ProfileDatabase +@TestPropertySource(properties = "logging.level.org.springframework.security=DEBUG") +@AddTestProperties +@AutoConfigureMockMvc +@TestMethodOrder(MethodOrderer.OrderAnnotation.class) +class AdminPendingVaultsControllerTest { + + @Autowired + Environment env; + + @MockitoBean + RestService restService; + + @Autowired + AdminPendingVaultsController adminPendingVaultsController; + + @Autowired + MockMvc mockMvc; + + @BeforeEach + final void setup() { + assertThat(adminPendingVaultsController).isNotNull(); + assertThat(env.getActiveProfiles()).containsExactly("database"); + } + + @Test + @Order(1) + @SneakyThrows + @WithMockUser(username = "vanilla-user", roles = {"USER"}) + void testDeletePendingVaultForbiddenForNonAdmins() { + + // Yes, delete pending vault users GET method + mockMvc.perform(get("/admin/pendingVaults/pendingVault123")).andDo(print()) + .andExpect(status().isForbidden()) + .andReturn(); + + verifyNoMoreInteractions(restService); + } + + @Test + @Order(2) + @SneakyThrows + @WithMockUser(username = "vanilla-user", roles = {"USER","IS_ADMIN"}) + void testDeletePendingVaultAllowsForSuperAdmins() { + + // Yes, delete pending vault users GET method + mockMvc.perform(get("/admin/pendingVaults/pendingVault123")).andDo(print()) + .andExpect(status().isFound()) + .andReturn(); + + verify(restService).deletePendingVault("pendingVault123"); + verifyNoMoreInteractions(restService); + + } +} \ No newline at end of file From fdf34cd00c43cc2ca90ad9445f9fd0b53a4ed7ad Mon Sep 17 00:00:00 2001 From: David Hay Date: Tue, 5 May 2026 12:01:43 +0100 Subject: [PATCH 5/7] LUC070-252-fix-urgent-pentest-findings - added more tests for security controls on /admin/pendingVaults/** for IS_ADMIN role --- .../admin/AdminPendingVaultsController.java | 8 +- .../AdminPendingVaultsControllerTest.java | 293 +++++++++++++++++- 2 files changed, 287 insertions(+), 14 deletions(-) diff --git a/datavault-webapp/src/main/java/org/datavaultplatform/webapp/controllers/admin/AdminPendingVaultsController.java b/datavault-webapp/src/main/java/org/datavaultplatform/webapp/controllers/admin/AdminPendingVaultsController.java index 55fd69d8b..f76fbdea7 100644 --- a/datavault-webapp/src/main/java/org/datavaultplatform/webapp/controllers/admin/AdminPendingVaultsController.java +++ b/datavault-webapp/src/main/java/org/datavaultplatform/webapp/controllers/admin/AdminPendingVaultsController.java @@ -33,7 +33,7 @@ public class AdminPendingVaultsController { private static final Logger logger = LoggerFactory.getLogger(AdminPendingVaultsController.class); - private static final int MAX_RECORDS_PER_PAGE = 10; + protected static final int MAX_RECORDS_PER_PAGE = 10; private final RestService restService; private final UserLookupService userLookupService; @@ -70,7 +70,7 @@ public String searchPendingVaults(ModelMap model, // The Admin Edit PV page @RequestMapping(value = "/admin/pendingVaults/edit/{vaultid}", method = RequestMethod.GET) - public String getPendingVault(ModelMap model, @PathVariable("vaultid") String vaultID) { + public String getPendingVaultForm(ModelMap model, @PathVariable("vaultid") String vaultID) { VaultInfo vault = restService.getPendingVault(vaultID); logger.info("Passed in id: '" + vaultID); model.addAttribute("vaultID", vaultID); @@ -157,7 +157,7 @@ private ModelMap createModelMap(ModelMap model, int filteredRecordsTotal, Vaults @RequestMapping(value = "/admin/pendingVaults/summary/{pendingVaultId}", method = RequestMethod.GET) - public String getVault(ModelMap model, @PathVariable("pendingVaultId") String vaultID, Principal principal) { + public String getVaultSummary(ModelMap model, @PathVariable("pendingVaultId") String vaultID, Principal principal) { logger.info("VaultID:'" + vaultID + "'"); VaultInfo pendingVault = restService.getPendingVault(vaultID); logger.info("pendingVault.id:'" + pendingVault.getID() + "'"); @@ -205,7 +205,7 @@ public String upgradeVault(@PathVariable("pendingVaultId") String pendingVaultID // Process the completed 'create new vault' page @RequestMapping(value = "/admin/pendingVaults/edit", method = RequestMethod.POST) - public String editPendingVault(@ModelAttribute CreateVault vault, ModelMap model, @RequestParam String action, + public String submitEditPendingVault(@ModelAttribute CreateVault vault, ModelMap model, @RequestParam String action, Principal principal) { // if the confirm button has been clicked save what we have if everything isn't // already saved and display the summary diff --git a/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminPendingVaultsControllerTest.java b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminPendingVaultsControllerTest.java index 92b739dd1..e5267eb75 100644 --- a/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminPendingVaultsControllerTest.java +++ b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/admin/AdminPendingVaultsControllerTest.java @@ -1,11 +1,18 @@ package org.datavaultplatform.webapp.controllers.admin; import lombok.SneakyThrows; +import org.datavaultplatform.common.model.Group; +import org.datavaultplatform.common.request.CreateVault; +import org.datavaultplatform.common.response.VaultInfo; +import org.datavaultplatform.common.response.VaultsData; import org.datavaultplatform.webapp.app.DataVaultWebApp; import org.datavaultplatform.webapp.services.RestService; +import org.datavaultplatform.webapp.services.UserLookupService; import org.datavaultplatform.webapp.test.AddTestProperties; import org.datavaultplatform.webapp.test.ProfileDatabase; import org.junit.jupiter.api.*; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; import org.springframework.boot.test.context.SpringBootTest; @@ -15,12 +22,16 @@ import org.springframework.test.context.bean.override.mockito.MockitoBean; import org.springframework.test.web.servlet.MockMvc; +import java.util.List; + import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.datavaultplatform.webapp.controllers.admin.AdminPendingVaultsController.MAX_RECORDS_PER_PAGE; +import static org.mockito.Mockito.*; +import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; @SuppressWarnings("DefaultAnnotationParam") @SpringBootTest(classes = DataVaultWebApp.class, webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) @@ -34,25 +45,287 @@ class AdminPendingVaultsControllerTest { @Autowired Environment env; - @MockitoBean - RestService restService; - @Autowired AdminPendingVaultsController adminPendingVaultsController; @Autowired MockMvc mockMvc; + @MockitoBean + RestService restService; + + @MockitoBean + UserLookupService userLookupService; + + @Captor + ArgumentCaptor argCreateVault; + @BeforeEach final void setup() { assertThat(adminPendingVaultsController).isNotNull(); assertThat(env.getActiveProfiles()).containsExactly("database"); } - + + @Test @Order(1) @SneakyThrows @WithMockUser(username = "vanilla-user", roles = {"USER"}) + void testSearchdPendingVaults_ForbiddenForNonAdmins() { + + mockMvc.perform(get("/admin/pendingVaults")).andDo(print()) + .andExpect(status().isForbidden()) + .andReturn(); + + verifyNoMoreInteractions(restService); + } + + @Test + @Order(2) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"USER", "IS_ADMIN"}) + void testSearchPendingVaults_AllowedForSuperAdmins() { + + VaultsData savedVaultsData = new VaultsData(); + savedVaultsData.setData(List.of()); + VaultsData confirmedVaultsData = new VaultsData(); + confirmedVaultsData.setData(List.of()); + + when(restService.searchPendingVaults(anyString(), anyString(), anyString(), anyInt(), anyInt(), anyBoolean())).thenReturn(savedVaultsData, confirmedVaultsData); + mockMvc.perform(get("/admin/pendingVaults")).andDo(print()) + .andExpect(view().name("admin/pendingVaults/index")) + .andReturn(); + + verify(restService, times(2)).searchPendingVaults(anyString(), anyString(), anyString(), anyInt(), eq(MAX_RECORDS_PER_PAGE), anyBoolean()); + verifyNoMoreInteractions(restService); + } + + @Test + @Order(3) + @SneakyThrows + @WithMockUser(username = "vanilla-user", roles = {"USER"}) + void testGetPendingVaultForm_ForbiddenForNonAdmins() { + + mockMvc.perform(get("/admin/pendingVaults/edit/pendingVaultId123")).andDo(print()) + .andExpect(status().isForbidden()) + .andReturn(); + + verifyNoMoreInteractions(restService); + } + + @Test + @Order(4) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"USER", "IS_ADMIN"}) + void testGetPendingVaultForm_AllowedForSuperAdmins() { + + VaultInfo mVaultInfo = mock(VaultInfo.class); + CreateVault mCreateVault = mock(CreateVault.class); + + when(mVaultInfo.convertToCreate()).thenReturn(mCreateVault); + + when(restService.getPendingVault("pendingVaultId123")).thenReturn(mVaultInfo); + mockMvc.perform(get("/admin/pendingVaults/edit/pendingVaultId123")).andDo(print()) + .andExpect(status().isOk()) + .andExpect(view().name("admin/pendingVaults/edit/editPendingVault")) + .andReturn(); + + verify(restService).getPendingVault("pendingVaultId123"); + verify(restService).getRetentionPolicyListing(); + verify(restService).getGroups(); + verify(mVaultInfo).convertToCreate(); + verifyNoMoreInteractions(restService, mVaultInfo); + } + + @Test + @Order(5) + @SneakyThrows + @WithMockUser(username = "vanilla-user", roles = {"USER"}) + void testSearchSavedPendingVaults_ForbiddenForNonAdmins() { + + mockMvc.perform(get("/admin/pendingVaults/saved")).andDo(print()) + .andExpect(status().isForbidden()) + .andReturn(); + + verifyNoMoreInteractions(restService); + } + + @Test + @Order(6) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"USER", "IS_ADMIN"}) + void testSearchSavedPendingVaults_AllowedForSuperAdmins() { + VaultsData filteredVaultsData = new VaultsData(); + filteredVaultsData.setData(List.of()); + + when(restService.searchPendingVaults(anyString(), anyString(), anyString(), anyInt(), anyInt(), anyBoolean())).thenReturn(filteredVaultsData); + mockMvc.perform(get("/admin/pendingVaults/saved")).andDo(print()) + .andExpect(view().name("admin/pendingVaults/saved")) + .andReturn(); + + verify(restService, times(1)).searchPendingVaults(anyString(), anyString(), anyString(), anyInt(), eq(MAX_RECORDS_PER_PAGE), eq(false)); + verifyNoMoreInteractions(restService); + } + + @Test + @Order(7) + @SneakyThrows + @WithMockUser(username = "vanilla-user", roles = {"USER"}) + void testSearchConfirmedPendingVaults_ForbiddenForNonAdmins() { + + mockMvc.perform(get("/admin/pendingVaults/confirmed")).andDo(print()) + .andExpect(status().isForbidden()) + .andReturn(); + + verifyNoMoreInteractions(restService); + } + + @Test + @Order(8) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"USER", "IS_ADMIN"}) + void testSearchConfirmedPendingVaults_AllowedForSuperAdmins() { + + VaultsData confirmedVaultsData = new VaultsData(); + confirmedVaultsData.setData(List.of()); + + when(restService.searchPendingVaults(anyString(), anyString(), anyString(), anyInt(), anyInt(), anyBoolean())).thenReturn(confirmedVaultsData); + mockMvc.perform(get("/admin/pendingVaults/confirmed")).andDo(print()) + .andExpect(view().name("admin/pendingVaults/confirmed")) + .andReturn(); + + verify(restService, times(1)).searchPendingVaults(anyString(), anyString(), anyString(), anyInt(), eq(MAX_RECORDS_PER_PAGE), eq(true)); + verifyNoMoreInteractions(restService); + } + + @Test + @Order(9) + @SneakyThrows + @WithMockUser(username = "vanilla-user", roles = {"USER"}) + void getGetVaultSummary_ForbiddenForNonAdmins() { + + mockMvc.perform(get("/admin/pendingVaults/summary/pendingVault123")).andDo(print()) + .andExpect(status().isForbidden()) + .andReturn(); + + verifyNoMoreInteractions(restService); + } + + @Test + @Order(10) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"USER", "IS_ADMIN"}) + void getGetVaultSummary_AllowedForSuperAdmins() { + + VaultInfo vaultInfo = new VaultInfo(); + vaultInfo.setID("pendingVault123"); + vaultInfo.setGroupID("group-id-123"); + + when(restService.getPendingVault("pendingVault123")).thenReturn(vaultInfo); + + Group group = new Group(); + group.setID("group-id-123"); + + when(restService.getGroup("group-id-123")).thenReturn(group); + + mockMvc.perform(get("/admin/pendingVaults/summary/pendingVault123")).andDo(print()) + .andExpect(status().isOk()) + .andExpect(view().name("admin/pendingVaults/summary")) + .andReturn(); + + verify(restService).getPendingVault("pendingVault123"); + verify(restService).getGroup("group-id-123"); + verifyNoMoreInteractions(restService); + } + + @Test + @Order(11) + @SneakyThrows + @WithMockUser(username = "vanilla-user", roles = {"USER"}) + void testUpgradeVault_ForbiddenForNonAdmins() { + + mockMvc.perform(get("/admin/pendingVaults/upgrade/pendingVault123")).andDo(print()) + .andExpect(status().isForbidden()) + .andReturn(); + + verifyNoMoreInteractions(restService); + } + + @Test + @Order(12) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"USER", "IS_ADMIN"}) + void testUpgradeVault_AllowedForSuperAdmins() { + + VaultInfo mVaultInfo1 = mock(VaultInfo.class); + VaultInfo mVaultInfo2 = mock(VaultInfo.class); + CreateVault mCreateVault = mock(CreateVault.class); + when(mVaultInfo1.convertToCreate()).thenReturn(mCreateVault); + when(mVaultInfo2.getID()).thenReturn("new-vault-id"); + when(mCreateVault.getPendingID()).thenReturn("pendingVault234"); + + when(restService.getPendingVault("pendingVault123")).thenReturn(mVaultInfo1); + when(restService.addVault(any(CreateVault.class))).thenReturn(mVaultInfo2); + + mockMvc.perform(get("/admin/pendingVaults/upgrade/pendingVault123?reviewDate=31-05-2035")).andDo(print()) + .andExpect(status().isFound()) + .andExpect(redirectedUrl("/vaults/new-vault-id/")); + + verify(restService).getPendingVault("pendingVault123"); + verify(restService).addVault(mCreateVault); + verify(restService).deletePendingVault("pendingVault234"); + + verifyNoMoreInteractions(restService, userLookupService); + } + + @Test + @Order(13) + @SneakyThrows + @WithMockUser(username = "vanilla-user", roles = {"USER"}) + void testSubmitEditPendingVault_ForbiddenForNonAdmins() { + mockMvc.perform(post("/admin/pendingVaults/edit").with(csrf())) + .andDo(print()) + .andExpect(status().isForbidden()) + .andReturn(); + + verifyNoMoreInteractions(restService); + + } + + @Test + @Order(14) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"USER", "IS_ADMIN"}) + void testSubmitEditPendingVault_AllowedForSuperAdmins() { + + VaultInfo vaultInfo = new VaultInfo(); + vaultInfo.setID("vault-info-id"); + + when(restService.editPendingVault(any(CreateVault.class))).thenReturn(vaultInfo); + + mockMvc.perform(post("/admin/pendingVaults/edit").with(csrf()) + .param("action", "the-action") + .param("pendingID", "pendingID123")) + .andDo(print()) + .andExpect(status().isFound()) + .andExpect(redirectedUrl("/admin/pendingVaults/edit/vault-info-id")) + .andReturn(); + + verify(userLookupService).checkNewRolesUserExists(argCreateVault.capture(), eq("/admin/pendingVaults/")); + + CreateVault actual = argCreateVault.getValue(); + assertThat(actual.getPendingID()).isEqualTo("pendingID123"); + + verify(restService).editPendingVault(actual); + + verifyNoMoreInteractions(restService, userLookupService); + } + + + @Test + @Order(15) + @SneakyThrows + @WithMockUser(username = "vanilla-user", roles = {"USER"}) void testDeletePendingVaultForbiddenForNonAdmins() { // Yes, delete pending vault users GET method @@ -64,16 +337,16 @@ void testDeletePendingVaultForbiddenForNonAdmins() { } @Test - @Order(2) + @Order(16) @SneakyThrows - @WithMockUser(username = "vanilla-user", roles = {"USER","IS_ADMIN"}) + @WithMockUser(username = "super-user", roles = {"USER", "IS_ADMIN"}) void testDeletePendingVaultAllowsForSuperAdmins() { // Yes, delete pending vault users GET method mockMvc.perform(get("/admin/pendingVaults/pendingVault123")).andDo(print()) .andExpect(status().isFound()) .andReturn(); - + verify(restService).deletePendingVault("pendingVault123"); verifyNoMoreInteractions(restService); From 3d45f956e2a44acf3af98cee0f8e540a08dc5cba Mon Sep 17 00:00:00 2001 From: David Hay Date: Tue, 5 May 2026 17:08:34 +0100 Subject: [PATCH 6/7] LUC070-252-fix-urgent-pentest-findings - added more tests for security controls on GET /vault/{vaultId}/{userId} and GET /vaults/autocompleteuun/{term} and GET /vaults/isuun/{uun} --- .../webapp/controllers/VaultsController.java | 19 +- .../controllers/VaultsControllerTest.java | 251 ++++++++++++++++++ 2 files changed, 266 insertions(+), 4 deletions(-) create mode 100644 datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/VaultsControllerTest.java diff --git a/datavault-webapp/src/main/java/org/datavaultplatform/webapp/controllers/VaultsController.java b/datavault-webapp/src/main/java/org/datavaultplatform/webapp/controllers/VaultsController.java index 95e3f15a1..36a6265e7 100644 --- a/datavault-webapp/src/main/java/org/datavaultplatform/webapp/controllers/VaultsController.java +++ b/datavault-webapp/src/main/java/org/datavaultplatform/webapp/controllers/VaultsController.java @@ -27,6 +27,7 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; +import org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.stereotype.Controller; @@ -308,10 +309,18 @@ private boolean canAccessVault(VaultInfo vault, Principal principal, Boolean pen || (RoleUtils.isRoleInSchool(roleAssignment, vault.getGroupID()) && RoleUtils.hasPermission(roleAssignment, Permission.CAN_MANAGE_VAULTS))); } } - @RequestMapping(value = "/vaults/{vaultid}/{userid}", method = RequestMethod.GET) - public String getVault(ModelMap model, @PathVariable("vaultid") String vaultID, @PathVariable("userid") String userID) { - model.addAttribute("vaults", restService.getVaultsListingAll(userID)); - + + @PreAuthorize("hasRole('IS_ADMIN') or #userId == authentication.name") + @GetMapping(value = "/vaults/{vaultId}/{userId}", produces = MediaType.TEXT_HTML_VALUE) + public String getVault(ModelMap model, @PathVariable String vaultId, @PathVariable String userId, Principal principal) { + VaultInfo vault = restService.getVault(vaultId); + if (vault == null) { + throw new EntityNotFoundException(Vault.class, vaultId); + } + if (!canAccessVault(vault, principal)) { + throw new ForbiddenException(); + } + model.addAttribute("vaults", restService.getVaultsListingAll(userId)); return "vaults/userVaults"; } @@ -525,6 +534,7 @@ public String updateVaultName(ModelMap model, return "redirect:" + vaultUrl; } + @PreAuthorize("hasRole('IS_ADMIN')") @RequestMapping(value = "/vaults/autocompleteuun/{term}", method = RequestMethod.GET) @ResponseBody public String autocompleteUUN(@PathVariable("term") String term) { @@ -533,6 +543,7 @@ public String autocompleteUUN(@PathVariable("term") String term) { return gson.toJson(result); } + @PreAuthorize("hasRole('IS_ADMIN')") @RequestMapping(value = "/vaults/isuun/{uun}", method = RequestMethod.GET) @ResponseBody public String isUUN(@PathVariable("uun") String uun) { diff --git a/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/VaultsControllerTest.java b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/VaultsControllerTest.java new file mode 100644 index 000000000..c2f1bfbcc --- /dev/null +++ b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/VaultsControllerTest.java @@ -0,0 +1,251 @@ +package org.datavaultplatform.webapp.controllers; + +import lombok.SneakyThrows; +import org.datavaultplatform.common.model.RoleAssignment; +import org.datavaultplatform.common.response.VaultInfo; +import org.datavaultplatform.common.util.RoleUtils; +import org.datavaultplatform.webapp.app.DataVaultWebApp; +import org.datavaultplatform.webapp.services.RestService; +import org.datavaultplatform.webapp.services.UserLookupService; +import org.datavaultplatform.webapp.test.AddTestProperties; +import org.datavaultplatform.webapp.test.ProfileDatabase; +import org.junit.jupiter.api.MethodOrderer; +import org.junit.jupiter.api.Order; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestMethodOrder; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.MockedStatic; +import org.mockito.Mockito; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.http.MediaType; +import org.springframework.security.test.context.support.WithMockUser; +import org.springframework.test.context.TestPropertySource; +import org.springframework.test.context.bean.override.mockito.MockitoBean; +import org.springframework.test.web.servlet.MockMvc; + +import java.util.List; + +import static org.mockito.Mockito.*; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; + +@SpringBootTest(classes = DataVaultWebApp.class, webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) +@ProfileDatabase +@TestPropertySource(properties = "logging.level.org.springframework.security=DEBUG") +@AddTestProperties +@AutoConfigureMockMvc +@TestMethodOrder(MethodOrderer.OrderAnnotation.class) +class VaultsControllerTest { + + @Autowired + MockMvc mockMvc; + @Captor + ArgumentCaptor roleAssignmentArg; + @MockitoBean + private RestService restService; + + @MockitoBean + private UserLookupService userLookupService; + + @Test + @Order(1) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"IS_ADMIN", "USER"}) + void testGetVault_FailsIfVaultNotFoundAsSuperUser() { + + when(restService.getVault("vault123")).thenReturn(null); + + mockMvc.perform(get("/vaults/vault123/user123")) + .andDo(print()).andExpect(status().isNotFound()); + + verify(restService).getVault("vault123"); + verifyNoMoreInteractions(restService, userLookupService); + + } + + @Test + @Order(2) + @SneakyThrows + @WithMockUser(username = "user123", roles = {"USER"}) + void testGetVault_FailsIfVaultNotFoundAsVanillaUser() { + + when(restService.getVault("vault123")).thenReturn(null); + + mockMvc.perform(get("/vaults/vault123/user123")) + .andDo(print()).andExpect(status().isNotFound()); + + verify(restService).getVault("vault123"); + verifyNoMoreInteractions(restService, userLookupService); + } + + @Test + @Order(3) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"IS_ADMIN", "USER"}) + void testGetVault_ForbiddenIfCannotAccessVaultAsSuperUser() { + + VaultInfo vaultInfo = new VaultInfo(); + when(restService.getVault("vault123")).thenReturn(vaultInfo); + + RoleAssignment ra1 = new RoleAssignment(); + when(restService.getRoleAssignmentsForUser("super-user")).thenReturn(List.of(ra1)); + + try (MockedStatic roleUtils = Mockito.mockStatic(RoleUtils.class)) { + roleUtils.when(() -> RoleUtils.isISAdmin(roleAssignmentArg.capture())).thenReturn(false); + + mockMvc.perform(get("/vaults/vault123/user123")) + .andDo(print()).andExpect(status().isForbidden()); + } + + verify(restService).getVault("vault123"); + verify(restService).getRoleAssignmentsForUser("super-user"); + verifyNoMoreInteractions(restService, userLookupService); + } + + @Test + @Order(4) + @SneakyThrows + @WithMockUser(username = "user123", roles = {"USER"}) + void testGetVault_ForbiddenIfCannotAccessVaultAsVanillaUser() { + + VaultInfo vaultInfo = new VaultInfo(); + when(restService.getVault("vault123")).thenReturn(vaultInfo); + + RoleAssignment ra1 = new RoleAssignment(); + when(restService.getRoleAssignmentsForUser("user123")).thenReturn(List.of(ra1)); + + try (MockedStatic roleUtils = Mockito.mockStatic(RoleUtils.class)) { + roleUtils.when(() -> RoleUtils.isISAdmin(roleAssignmentArg.capture())).thenReturn(false); + + mockMvc.perform(get("/vaults/vault123/user123")) + .andDo(print()).andExpect(status().isForbidden()); + } + + verify(restService).getVault("vault123"); + verify(restService).getRoleAssignmentsForUser("user123"); + verifyNoMoreInteractions(restService, userLookupService); + + } + + @Test + @Order(5) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"IS_ADMIN", "USER"}) + void testGetVault_AllowedIfCanAccessVaultAsSuperUser() { + + VaultInfo vaultInfo = new VaultInfo(); + when(restService.getVault("vault123")).thenReturn(vaultInfo); + + RoleAssignment ra1 = new RoleAssignment(); + when(restService.getRoleAssignmentsForUser("super-user")).thenReturn(List.of(ra1)); + + VaultInfo[] vaultListing = new VaultInfo[0]; + when(restService.getVaultsListingAll("user123")).thenReturn(vaultListing); + + try (MockedStatic roleUtils = Mockito.mockStatic(RoleUtils.class)) { + roleUtils.when(() -> RoleUtils.isISAdmin(roleAssignmentArg.capture())).thenReturn(true); + + mockMvc.perform(get("/vaults/vault123/user123")) + .andDo(print()) + .andExpect(model().attribute("vaults", vaultListing)) + .andExpect(view().name("vaults/userVaults")); + } + + verify(restService).getVault("vault123"); + verify(restService).getRoleAssignmentsForUser("super-user"); + verify(restService).getVaultsListingAll("user123"); + verifyNoMoreInteractions(restService, userLookupService); + + } + + @Test + @Order(6) + @SneakyThrows + @WithMockUser(username = "user123", roles = {"USER"}) + void testGetVault_AllowedIfCanAccessVaultAsVanillaUser() { + + VaultInfo vaultInfo = new VaultInfo(); + when(restService.getVault("vault123")).thenReturn(vaultInfo); + + RoleAssignment ra1 = new RoleAssignment(); + when(restService.getRoleAssignmentsForUser("user123")).thenReturn(List.of(ra1)); + + VaultInfo[] vaultListing = new VaultInfo[0]; + when(restService.getVaultsListingAll("user123")).thenReturn(vaultListing); + + try (MockedStatic roleUtils = Mockito.mockStatic(RoleUtils.class)) { + roleUtils.when(() -> RoleUtils.isISAdmin(roleAssignmentArg.capture())).thenReturn(true); + + mockMvc.perform(get("/vaults/vault123/user123")) + .andDo(print()) + .andExpect(model().attribute("vaults", vaultListing)) + .andExpect(view().name("vaults/userVaults")); + } + + verify(restService).getVault("vault123"); + verify(restService).getRoleAssignmentsForUser("user123"); + verify(restService).getVaultsListingAll("user123"); + verifyNoMoreInteractions(restService, userLookupService); + + } + + @Test + @Order(7) + @SneakyThrows + @WithMockUser(username = "vanilla-user", roles = {"USER"}) + void testIsUUN_ForbiddenAsVanillaUser() { + mockMvc.perform(get("/vaults/isuun/v1dhay3")).andExpect(status().isForbidden()); + verifyNoMoreInteractions(restService, userLookupService); + + } + + @Order(8) + @SneakyThrows + @ParameterizedTest + @ValueSource(booleans = {true, false}) + @WithMockUser(username = "super-user", roles = {"USER","IS_ADMIN"}) + void testIsUUN_AllowedAsSuperUser(boolean isUUN) { + when(userLookupService.isUUN("v1dhay3")).thenReturn(isUUN); + mockMvc.perform(get("/vaults/isuun/v1dhay3")) + .andExpect(status().isOk()) + .andExpect(content().string(String.valueOf(isUUN))) + .andExpect(content().contentTypeCompatibleWith(MediaType.TEXT_PLAIN)); + + verify(userLookupService).isUUN("v1dhay3"); + verifyNoMoreInteractions(restService, userLookupService); + } + + @Test + @Order(9) + @SneakyThrows + @WithMockUser(username = "vanilla-user", roles = {"USER"}) + void testAutocompleteUUN_ForbiddenAsVanillaUser() { + mockMvc.perform(get("/vaults/autocompleteuun/blah")).andExpect(status().isForbidden()); + verifyNoMoreInteractions(restService, userLookupService); + } + + + @Test + @Order(10) + @SneakyThrows + @WithMockUser(username = "super-user", roles = {"USER","IS_ADMIN"}) + void testAutocompleteUUN_AllowedAsSuperUser() { + when(userLookupService.getSuggestedUuns("blah")).thenReturn(List.of("blah1","blah2","blah3")); + mockMvc.perform(get("/vaults/autocompleteuun/blah")) + .andExpect(status().isOk()) + .andExpect(content().string( + """ + ["blah1","blah2","blah3"]""")) + .andExpect(content().contentTypeCompatibleWith(MediaType.TEXT_PLAIN)); + + verify(userLookupService).getSuggestedUuns("blah"); + verifyNoMoreInteractions(restService, userLookupService); + } + +} \ No newline at end of file From 5e363d893c2b6627d392012ebf6a908ad5497cc9 Mon Sep 17 00:00:00 2001 From: David Hay Date: Wed, 6 May 2026 09:30:00 +0100 Subject: [PATCH 7/7] LUC070-252-fix-urgent-pentest-findings - renamed VaultsControllerTest to VaultsControllerMvcTest --- .../{VaultsControllerTest.java => VaultsControllerMvcTest.java} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/{VaultsControllerTest.java => VaultsControllerMvcTest.java} (99%) diff --git a/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/VaultsControllerTest.java b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/VaultsControllerMvcTest.java similarity index 99% rename from datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/VaultsControllerTest.java rename to datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/VaultsControllerMvcTest.java index c2f1bfbcc..0d6be6534 100644 --- a/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/VaultsControllerTest.java +++ b/datavault-webapp/src/test/java/org/datavaultplatform/webapp/controllers/VaultsControllerMvcTest.java @@ -41,7 +41,7 @@ @AddTestProperties @AutoConfigureMockMvc @TestMethodOrder(MethodOrderer.OrderAnnotation.class) -class VaultsControllerTest { +class VaultsControllerMvcTest { @Autowired MockMvc mockMvc;