diff --git a/src/main/java/it/inaf/oats/vospace/AbstractNodeService.java b/src/main/java/it/inaf/oats/vospace/AbstractNodeService.java index c66e1fb29232aa3b789d9762a20eae55ed0c138e..2847bf4e66b06f3c9177aeb5d8dbd3ad71f824fa 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 c12ea6c1e6888e58318eb1d6836fa620bdf95428..bba6b6be692c263338cf2dc85c1d67a0fea14ad6 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 fb87a2792333efa6e51b3575b2b15bab197b1310..2eb3b872653fa2aa6229de37876239f9ebc34190 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 e7d186eddf0ed5513e3d588010e505ab78089e83..e1927ab0a44a4cbc8210ba580dc71c94df8677c3 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 48d05de50e46e3a692ed059ed22bbe77a104495e..729657f3827c2b5ffc855dd171299059c6e460ae 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 69d342c85e97a9b94972f4955e9262b01d2f13bf..84c8a22937fa215c00382374c6ae6589513b586c 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 eae2766d31afcb6b31b11e1ed07e6f92a113f1bc..132d0b737298165f2539153474c18dd6a804645c 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