From b990aebb769fcebfc2d197bdc92b5e670814cf46 Mon Sep 17 00:00:00 2001 From: Nicola Fulvio Calabria Date: Sun, 27 Jun 2021 17:54:47 +0200 Subject: [PATCH] Added some tests --- .../oats/vospace/AbstractNodeService.java | 3 - .../it/inaf/oats/vospace/CopyService.java | 2 +- .../inaf/oats/vospace/NodeBranchService.java | 10 +- .../it/inaf/oats/vospace/CopyServiceTest.java | 206 +++++++++++++++--- .../it/inaf/oats/vospace/MoveServiceTest.java | 3 - .../oats/vospace/NodeBranchServiceTest.java | 18 +- .../oats/vospace/persistence/NodeDAOTest.java | 2 +- 7 files changed, 185 insertions(+), 59 deletions(-) diff --git a/src/main/java/it/inaf/oats/vospace/AbstractNodeService.java b/src/main/java/it/inaf/oats/vospace/AbstractNodeService.java index c66e1fb..2847bf4 100644 --- a/src/main/java/it/inaf/oats/vospace/AbstractNodeService.java +++ b/src/main/java/it/inaf/oats/vospace/AbstractNodeService.java @@ -21,9 +21,6 @@ public abstract class AbstractNodeService { @Value("${vospace-authority}") protected String authority; - - @Autowired - protected HttpServletRequest servletRequest; protected void validatePath(String path) { if (path.equals("/")) { diff --git a/src/main/java/it/inaf/oats/vospace/CopyService.java b/src/main/java/it/inaf/oats/vospace/CopyService.java index c12ea6c..bba6b6b 100644 --- a/src/main/java/it/inaf/oats/vospace/CopyService.java +++ b/src/main/java/it/inaf/oats/vospace/CopyService.java @@ -50,7 +50,7 @@ public class CopyService extends AbstractNodeService { try { // check source branch for read and lock it - nodeBranchService.checkBranchForReadAndLock(sourcePath, jobId); + nodeBranchService.checkBranchForReadAndLock(sourcePath, jobId, user); // Check destination Optional destShortNodeDescriptor diff --git a/src/main/java/it/inaf/oats/vospace/NodeBranchService.java b/src/main/java/it/inaf/oats/vospace/NodeBranchService.java index fb87a27..2eb3b87 100644 --- a/src/main/java/it/inaf/oats/vospace/NodeBranchService.java +++ b/src/main/java/it/inaf/oats/vospace/NodeBranchService.java @@ -33,15 +33,9 @@ public class NodeBranchService { @Autowired private NodeDAO nodeDao; - @Autowired - private HttpServletRequest servletRequest; - @Transactional(rollbackFor = {Exception.class}, isolation = Isolation.REPEATABLE_READ, propagation = Propagation.REQUIRED) - public void checkBranchForReadAndLock(String sourcePath, String jobId) { - - // Extract User permissions from servlet request - User user = (User) servletRequest.getUserPrincipal(); + public void checkBranchForReadAndLock(String sourcePath, String jobId, User user) { try { // Get source node @@ -52,7 +46,7 @@ public class NodeBranchService { throw new NodeBusyException(sourcePath); } - if (!nodeDao.isBranchReadable(sourceId, user.getName(), user.getGroups())) { + if (!nodeDao.isBranchReadable(sourceId, user.getName(), user.getGroups())) { throw new PermissionDeniedException(sourcePath); } diff --git a/src/test/java/it/inaf/oats/vospace/CopyServiceTest.java b/src/test/java/it/inaf/oats/vospace/CopyServiceTest.java index e7d186e..e1927ab 100644 --- a/src/test/java/it/inaf/oats/vospace/CopyServiceTest.java +++ b/src/test/java/it/inaf/oats/vospace/CopyServiceTest.java @@ -14,9 +14,7 @@ import it.inaf.oats.vospace.persistence.NodeDAO; import java.util.Arrays; import java.util.List; import java.util.Optional; -import javax.servlet.http.HttpServletRequest; import net.ivoa.xml.vospace.v2.Transfer; -import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import org.junit.jupiter.api.MethodOrderer.OrderAnnotation; @@ -29,16 +27,14 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; import org.springframework.boot.test.context.SpringBootTest; -import org.springframework.boot.test.context.TestConfiguration; -import org.springframework.context.annotation.Bean; -import org.springframework.context.annotation.Primary; +import org.springframework.dao.DuplicateKeyException; import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.context.TestPropertySource; import org.springframework.test.context.ContextConfiguration; @SpringBootTest @AutoConfigureMockMvc -@ContextConfiguration(classes = {DataSourceConfigSingleton.class, CopyServiceTest.TestConfig.class}) +@ContextConfiguration(classes = DataSourceConfigSingleton.class) @TestPropertySource(locations = "classpath:test.properties", properties = {"vospace-authority=example.com!vospace", "file-service-url=http://file-service"}) @TestMethodOrder(OrderAnnotation.class) @DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_EACH_TEST_METHOD) @@ -46,31 +42,187 @@ public class CopyServiceTest { @Value("${vospace-authority}") private String authority; - + @Autowired - private MoveService moveService; - + private CopyService copyService; + @Autowired private NodeDAO nodeDao; - @Autowired - private HttpServletRequest servletRequest; - - @TestConfiguration - public static class TestConfig { - - /** - * Necessary because MockBean doesn't work with HttpServletRequest. - */ - @Bean - @Primary - public HttpServletRequest servletRequest() { - HttpServletRequest request = mock(HttpServletRequest.class); - User user = new User().setUserId("anonymous"); - when(request.getUserPrincipal()).thenReturn(user); - return request; + @Test + @Order(1) + public void copyRootTest() { + + assertThrows(IllegalArgumentException.class, () -> { + copyService.processCopyNodes(getTransfer("/", "/pippo"), "job_pippo", getAnonymousUser()); + } + ); + + assertThrows(IllegalArgumentException.class, () -> { + copyService.processCopyNodes(getTransfer("/pippo", "/"), "job_pippo", getAnonymousUser()); + } + ); + + } + + @Test + @Order(2) + public void testNonExistingSourceNode() { + assertThrows(NodeNotFoundException.class, () -> { + copyService.processCopyNodes(getTransfer("/pippo", "/test2"), "job_pippo", getAnonymousUser()); + } + ); + } + + @Test + @Order(3) + public void testCopyDeniedOnBusySource() { + User user = mock(User.class); + when(user.getName()).thenReturn("user3"); + + assertThrows(NodeBusyException.class, () -> { + copyService.processCopyNodes(getTransfer("/test3/mbusy", "/test3/m1"), "job_pippo", user); + } + ); + } + + @Test + @Order(4) + public void testPermissionDeniedOnSource() { + User user = mock(User.class); + when(user.getName()).thenReturn("user1"); + + assertThrows(PermissionDeniedException.class, () -> { + copyService.processCopyNodes(getTransfer("/test3/m1", "/test4"), "job_pippo", user); + } + ); + } + + @Test + @Order(5) + public void testDontMoveIfStickySource() { + User user = mock(User.class); + when(user.getName()).thenReturn("user3"); + + assertThrows(PermissionDeniedException.class, () -> { + copyService.processCopyNodes(getTransfer("/test3/mstick", "/test4"), "job_pippo", user); + } + ); + } + + @Test + @Order(6) + public void testPermissionDeniedOnExistingDestination() { + User user = mock(User.class); + when(user.getName()).thenReturn("user1"); + when(user.getGroups()).thenReturn(List.of("group1")); + + assertThrows(PermissionDeniedException.class, () -> { + copyService.processCopyNodes(getTransfer("/test3/group1", "/test3/m1/m2"), "job_pippo", user); + } + ); + } + + @Test + @Order(7) + public void testDestinationExistsAndIsBusy() { + User user = mock(User.class); + when(user.getName()).thenReturn("user3"); + + assertThrows(NodeBusyException.class, () -> { + copyService.processCopyNodes(getTransfer("/test3/m1", "/test3/mbusy"), "job_pippo", user); + } + ); + } + + @Test + @Order(9) + public void testCopyToExistingDestination() { + User user = mock(User.class); + when(user.getName()).thenReturn("user3"); + + // Preliminary checks for assumptions + Optional sourceId = nodeDao.getNodeId("/test3/m1"); + assertTrue(sourceId.isPresent()); + Optional childId = nodeDao.getNodeId("/test3/m1/m2"); + assertTrue(childId.isPresent()); + + Optional destId = nodeDao.getNodeId("/test4"); + assertTrue(destId.isPresent()); + + // copy + copyService.processCopyNodes(getTransfer("/test3/m1", "/test4"), "job_pippo", user); + + // source has been moved + Optional oldSourceId = nodeDao.getNodeId("/test3/m1"); + assertTrue(oldSourceId.isPresent()); + Optional oldChildId = nodeDao.getNodeId("/test3/m1/m2"); + assertTrue(oldChildId.isPresent()); + + Optional newSourceId = nodeDao.getNodeId("/test4/m1"); + assertTrue(newSourceId.isPresent()); + + Optional newChildId = nodeDao.getNodeId("/test4/m1/m2"); + assertTrue(newChildId.isPresent()); + + } + + @Test + @Order(10) + public void testCopyToExistingParent() { + User user = mock(User.class); + when(user.getName()).thenReturn("user3"); + + // Preliminary checks for assumptions + Optional sourceId = nodeDao.getNodeId("/test3/m1"); + assertTrue(sourceId.isPresent()); + Optional childId = nodeDao.getNodeId("/test3/m1/m2"); + assertTrue(childId.isPresent()); + + Optional destId = nodeDao.getNodeId("/test3/m1/m2_copy"); + assertTrue(destId.isEmpty()); + + // copy + copyService.processCopyNodes(getTransfer("/test3/m1/m2", "/test3/m1/m2_copy"), "job_pippo", user); + + // source has been moved + Optional oldSourceId = nodeDao.getNodeId("/test3/m1"); + assertTrue(oldSourceId.isPresent()); + Optional oldChildId = nodeDao.getNodeId("/test3/m1/m2"); + assertTrue(oldChildId.isPresent()); + + Optional newSourceId = nodeDao.getNodeId("/test3/m1/m2_copy"); + assertTrue(newSourceId.isPresent()); + + } + + @Test + @Order(11) + public void testCopyDeniedToExistingDestination() { + + User user = mock(User.class); + when(user.getName()).thenReturn("user3"); + + // Preliminary checks for assumptions + Optional sourceId = nodeDao.getNodeId("/test3/m1"); + assertTrue(sourceId.isPresent()); + Optional childId = nodeDao.getNodeId("/test3/m1/m2"); + assertTrue(childId.isPresent()); + + assertThrows(DuplicateKeyException.class, () -> { + copyService.processCopyNodes(getTransfer("/test3/m1/m2", "/test3/m1"), "job_pippo", user); } + ); + } + + private Transfer getTransfer(String vosTarget, String vosDestination) { + Transfer transfer = new Transfer(); + transfer.setTarget(Arrays.asList("vos://" + this.authority + vosTarget)); + transfer.setDirection("vos://" + this.authority + vosDestination); + return transfer; + } + + private User getAnonymousUser() { + return new User().setUserId("anonymous"); } - - // Stub Test Class } diff --git a/src/test/java/it/inaf/oats/vospace/MoveServiceTest.java b/src/test/java/it/inaf/oats/vospace/MoveServiceTest.java index 48d05de..729657f 100644 --- a/src/test/java/it/inaf/oats/vospace/MoveServiceTest.java +++ b/src/test/java/it/inaf/oats/vospace/MoveServiceTest.java @@ -188,9 +188,6 @@ public class MoveServiceTest { Optional destParentId = nodeDao.getNodeId("/test4"); assertTrue(destParentId.isPresent()); - Optional destId = nodeDao.getNodeId("/test4"); - assertTrue(destId.isPresent()); - // move moveService.processMoveJob(getTransfer("/test3/m1", "/test4"), user); diff --git a/src/test/java/it/inaf/oats/vospace/NodeBranchServiceTest.java b/src/test/java/it/inaf/oats/vospace/NodeBranchServiceTest.java index 69d342c..84c8a22 100644 --- a/src/test/java/it/inaf/oats/vospace/NodeBranchServiceTest.java +++ b/src/test/java/it/inaf/oats/vospace/NodeBranchServiceTest.java @@ -49,22 +49,8 @@ public class NodeBranchServiceTest { @Autowired private NodeDAO nodeDao; - @Test - @Order(1) - public void moveRootTest() { - - assertThrows(IllegalArgumentException.class, () -> { - moveService.processMoveJob(getTransfer("/", "/pippo"), getAnonymousUser()); - } - ); - - assertThrows(IllegalArgumentException.class, () -> { - moveService.processMoveJob(getTransfer("/pippo", "/"), getAnonymousUser()); - } - ); - - } - + // Stub test class + private Transfer getTransfer(String vosTarget, String vosDestination) { Transfer transfer = new Transfer(); transfer.setTarget(Arrays.asList("vos://" + this.authority + vosTarget)); diff --git a/src/test/java/it/inaf/oats/vospace/persistence/NodeDAOTest.java b/src/test/java/it/inaf/oats/vospace/persistence/NodeDAOTest.java index eae2766..132d0b7 100644 --- a/src/test/java/it/inaf/oats/vospace/persistence/NodeDAOTest.java +++ b/src/test/java/it/inaf/oats/vospace/persistence/NodeDAOTest.java @@ -229,7 +229,7 @@ public class NodeDAOTest { optId = dao.getNodeId("/test3/group1"); assertTrue(optId.isPresent()); - assertFalse(dao.isBranchWritable(optId.get(), "user1", List.of("group99"))); + assertFalse(dao.isBranchReadable(optId.get(), "user1", List.of("group99"))); } @Test -- GitLab