From 0babd77f1c58bb3de283ace05bf9c292a023cea8 Mon Sep 17 00:00:00 2001
From: Nicola Fulvio Calabria <nicola.calabria@inaf.it>
Date: Sat, 31 Jul 2021 11:20:36 +0200
Subject: [PATCH] Minor refactoring + updated constraints to copy paths

---
 .../it/inaf/oats/vospace/CopyService.java     | 35 +++++++--
 .../it/inaf/oats/vospace/MoveService.java     |  2 +-
 .../inaf/oats/vospace/NodeBranchService.java  | 71 -------------------
 3 files changed, 29 insertions(+), 79 deletions(-)
 delete mode 100644 src/main/java/it/inaf/oats/vospace/NodeBranchService.java

diff --git a/src/main/java/it/inaf/oats/vospace/CopyService.java b/src/main/java/it/inaf/oats/vospace/CopyService.java
index 47f12cd..e26ece0 100644
--- a/src/main/java/it/inaf/oats/vospace/CopyService.java
+++ b/src/main/java/it/inaf/oats/vospace/CopyService.java
@@ -7,12 +7,12 @@ package it.inaf.oats.vospace;
 
 import it.inaf.ia2.aa.data.User;
 import it.inaf.oats.vospace.datamodel.NodeUtils;
-import it.inaf.oats.vospace.exception.InvalidArgumentException;
 import it.inaf.oats.vospace.exception.NodeBusyException;
+import it.inaf.oats.vospace.exception.NodeNotFoundException;
+import it.inaf.oats.vospace.exception.PermissionDeniedException;
 import it.inaf.oats.vospace.persistence.NodeDAO.ShortNodeDescriptor;
 import java.util.Optional;
 import net.ivoa.xml.vospace.v2.Transfer;
-import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.dao.CannotSerializeTransactionException;
 import org.springframework.stereotype.Service;
 import org.springframework.transaction.annotation.EnableTransactionManagement;
@@ -23,9 +23,6 @@ import org.springframework.transaction.annotation.Transactional;
 @EnableTransactionManagement
 public class CopyService extends AbstractNodeService {
 
-    @Autowired
-    private NodeBranchService nodeBranchService;
-
     @Transactional(rollbackFor = {Exception.class}, isolation = Isolation.REPEATABLE_READ)
     public String processCopyNodes(Transfer transfer, String jobId, User user) {
 
@@ -42,13 +39,19 @@ public class CopyService extends AbstractNodeService {
         this.validatePath(destinationPath);
 
         if (sourcePath.equals(destinationPath)) {
-            return null;
+            throw new IllegalArgumentException("Cannot copy node to itself");
+        }
+
+        // Check if destination is subpath of source
+        // Linux-like: "cannot copy to a subdirectory of itself" 
+        if (destinationPath.startsWith(sourcePath + "/")) {
+            throw new IllegalArgumentException("Cannot copy node to a subdirectory of its own path");
         }
 
         try {
 
             // check source branch for read and lock it
-            nodeBranchService.checkBranchForReadAndLock(sourcePath, jobId, user);
+            this.checkBranchForReadAndLock(sourcePath, jobId, user);
 
             // Check destination
             Optional<ShortNodeDescriptor> destShortNodeDescriptor
@@ -86,4 +89,22 @@ public class CopyService extends AbstractNodeService {
 
     }
 
+    private void checkBranchForReadAndLock(String sourcePath, String jobId, User user) {
+
+        // Get source node
+        Optional<Long> sourceIdOpt = nodeDao.getNodeId(sourcePath);
+        long sourceId = sourceIdOpt.orElseThrow(() -> new NodeNotFoundException(sourcePath));
+
+        if (nodeDao.isBranchBusy(sourceId)) {
+            throw new NodeBusyException(sourcePath);
+        }
+
+        if (!nodeDao.isBranchReadable(sourceId, user.getName(), user.getGroups())) {
+            throw new PermissionDeniedException(sourcePath);
+        }
+
+        nodeDao.setBranchJobId(sourceId, jobId);
+
+    }
+
 }
diff --git a/src/main/java/it/inaf/oats/vospace/MoveService.java b/src/main/java/it/inaf/oats/vospace/MoveService.java
index c852a0f..0a7b784 100644
--- a/src/main/java/it/inaf/oats/vospace/MoveService.java
+++ b/src/main/java/it/inaf/oats/vospace/MoveService.java
@@ -43,7 +43,7 @@ public class MoveService extends AbstractNodeService {
         this.validatePath(destinationPath);
 
         if (sourcePath.equals(destinationPath)) {
-            return;
+            throw new IllegalArgumentException("Cannot move node to itself");
         }
         
         // Check if destination is subpath of source
diff --git a/src/main/java/it/inaf/oats/vospace/NodeBranchService.java b/src/main/java/it/inaf/oats/vospace/NodeBranchService.java
deleted file mode 100644
index 61d64ff..0000000
--- a/src/main/java/it/inaf/oats/vospace/NodeBranchService.java
+++ /dev/null
@@ -1,71 +0,0 @@
-/*
- * This file is part of vospace-rest
- * Copyright (C) 2021 Istituto Nazionale di Astrofisica
- * SPDX-License-Identifier: GPL-3.0-or-later
- */
-package it.inaf.oats.vospace;
-
-import it.inaf.ia2.aa.data.User;
-import it.inaf.oats.vospace.datamodel.NodeUtils;
-import it.inaf.oats.vospace.exception.InternalFaultException;
-import it.inaf.oats.vospace.exception.InvalidArgumentException;
-import it.inaf.oats.vospace.exception.NodeBusyException;
-import it.inaf.oats.vospace.exception.NodeNotFoundException;
-import it.inaf.oats.vospace.exception.PermissionDeniedException;
-import it.inaf.oats.vospace.persistence.NodeDAO;
-import it.inaf.oats.vospace.persistence.NodeDAO.ShortNodeDescriptor;
-import java.util.Optional;
-import javax.servlet.http.HttpServletRequest;
-import net.ivoa.xml.vospace.v2.Transfer;
-import org.springframework.beans.factory.annotation.Autowired;
-import org.springframework.beans.factory.annotation.Value;
-import org.springframework.dao.CannotSerializeTransactionException;
-import org.springframework.stereotype.Service;
-import org.springframework.transaction.annotation.EnableTransactionManagement;
-import org.springframework.transaction.annotation.Isolation;
-import org.springframework.transaction.annotation.Propagation;
-import org.springframework.transaction.annotation.Transactional;
-
-@Service
-@EnableTransactionManagement
-public class NodeBranchService {
-
-    @Autowired
-    private NodeDAO nodeDao;
-
-    @Transactional(rollbackFor = {Exception.class}, isolation = Isolation.REPEATABLE_READ,
-            propagation = Propagation.REQUIRED)
-    public void checkBranchForReadAndLock(String sourcePath, String jobId, User user) {
-
-        try {
-            // Get source node
-            Optional<Long> sourceIdOpt = nodeDao.getNodeId(sourcePath);
-            long sourceId = sourceIdOpt.orElseThrow(() -> new NodeNotFoundException(sourcePath));
-
-            if (nodeDao.isBranchBusy(sourceId)) {
-                throw new NodeBusyException(sourcePath);
-            }
-
-            if (!nodeDao.isBranchReadable(sourceId, user.getName(), user.getGroups())) {                
-                throw new PermissionDeniedException(sourcePath);
-            }
-            
-            this.lockBranch(sourceId, jobId);
-
-        } catch (CannotSerializeTransactionException ex) {
-            // Concurrent transactions attempted to modify this set of nodes            
-            throw new NodeBusyException(sourcePath);
-        }
-    } 
-    
-    private void lockBranch(Long sourceRootId, String jobId) {
-
-        nodeDao.setBranchJobId(sourceRootId, jobId);
-
-    }
-    
-    public void unlockNodes(String jobId) {
-        nodeDao.releaseBusyNodesByJobId(jobId);
-    }
-
-}
-- 
GitLab