From 9a87b926afa8ad4c79aec8bdd8cc31355d572043 Mon Sep 17 00:00:00 2001
From: Sonia Zorba <sonia.zorba@inaf.it>
Date: Fri, 12 Feb 2021 15:53:04 +0100
Subject: [PATCH] Handled special chars and added some logging

---
 .../inaf/oats/vospace/BaseNodeController.java | 15 +++++------
 .../oats/vospace/CreateNodeController.java    | 13 +++++----
 .../oats/vospace/DeleteNodeController.java    | 18 +++++--------
 .../inaf/oats/vospace/ListNodeController.java |  5 ++++
 .../java/it/inaf/oats/vospace/UriService.java |  8 +++---
 .../exception/InvalidURIException.java        | 15 ++++++-----
 .../vospace/CreateNodeControllerTest.java     | 27 ++++++++++++++-----
 7 files changed, 56 insertions(+), 45 deletions(-)

diff --git a/src/main/java/it/inaf/oats/vospace/BaseNodeController.java b/src/main/java/it/inaf/oats/vospace/BaseNodeController.java
index a9661a1..d612a56 100644
--- a/src/main/java/it/inaf/oats/vospace/BaseNodeController.java
+++ b/src/main/java/it/inaf/oats/vospace/BaseNodeController.java
@@ -1,6 +1,7 @@
 package it.inaf.oats.vospace;
 
 import it.inaf.oats.vospace.datamodel.NodeUtils;
+import it.inaf.oats.vospace.exception.InvalidURIException;
 import javax.servlet.http.HttpServletRequest;
 import org.springframework.beans.factory.annotation.Autowired;
 
@@ -9,20 +10,16 @@ public abstract class BaseNodeController {
     @Autowired
     private HttpServletRequest servletRequest;
 
-    
     protected String getPath() {
-        
         String requestURL = servletRequest.getRequestURL().toString();
-        
-        return NodeUtils.getPathFromRequestURLString(requestURL);
-        
+        try {
+            return NodeUtils.getPathFromRequestURLString(requestURL);
+        } catch (IllegalArgumentException ex) {
+            throw new InvalidURIException(ex);
+        }
     }
-    
-    
 
     protected String getParentPath(String path) {
-
         return NodeUtils.getParentPath(path);
     }
-    
 }
diff --git a/src/main/java/it/inaf/oats/vospace/CreateNodeController.java b/src/main/java/it/inaf/oats/vospace/CreateNodeController.java
index 488d2c1..b325061 100644
--- a/src/main/java/it/inaf/oats/vospace/CreateNodeController.java
+++ b/src/main/java/it/inaf/oats/vospace/CreateNodeController.java
@@ -1,7 +1,6 @@
 package it.inaf.oats.vospace;
 
 import it.inaf.ia2.aa.data.User;
-import it.inaf.oats.vospace.datamodel.NodeProperties;
 import it.inaf.oats.vospace.datamodel.NodeUtils;
 import net.ivoa.xml.vospace.v2.Node;
 import org.springframework.http.MediaType;
@@ -9,15 +8,17 @@ import org.springframework.web.bind.annotation.RequestBody;
 import org.springframework.web.bind.annotation.RestController;
 import org.springframework.beans.factory.annotation.Autowired;
 import it.inaf.oats.vospace.persistence.NodeDAO;
-import java.util.List;
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.web.bind.annotation.PutMapping;
 import it.inaf.oats.vospace.exception.*;
-import java.util.stream.Collectors;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 @RestController
 public class CreateNodeController extends BaseNodeController {
 
+    private static final Logger LOG = LoggerFactory.getLogger(CreateNodeController.class);
+    
     @Autowired
     private NodeDAO nodeDao;
 
@@ -30,6 +31,8 @@ public class CreateNodeController extends BaseNodeController {
     public Node createNode(@RequestBody Node node, User principal) {
 
         String path = getPath();
+        
+        LOG.debug("createNode called for path {}", path);
 
         // Validate payload node URI
         if (!isValidURI(node.getUri())) {
@@ -93,9 +96,5 @@ public class CreateNodeController extends BaseNodeController {
         // form vos://authority[!~]somepath/mynode..."
 
         return nodeURI.replaceAll("vos://[^/]+", "").equals(path);
-
     }
-
-    
-
 }
diff --git a/src/main/java/it/inaf/oats/vospace/DeleteNodeController.java b/src/main/java/it/inaf/oats/vospace/DeleteNodeController.java
index eae1a86..168700a 100644
--- a/src/main/java/it/inaf/oats/vospace/DeleteNodeController.java
+++ b/src/main/java/it/inaf/oats/vospace/DeleteNodeController.java
@@ -1,36 +1,29 @@
-/*
- * To change this license header, choose License Headers in Project Properties.
- * To change this template file, choose Tools | Templates
- * and open the template in the editor.
- */
 package it.inaf.oats.vospace;
 
 import it.inaf.ia2.aa.data.User;
-import it.inaf.oats.vospace.datamodel.NodeProperties;
 import it.inaf.oats.vospace.datamodel.NodeUtils;
-import it.inaf.oats.vospace.exception.ContainerNotFoundException;
-import it.inaf.oats.vospace.exception.InternalFaultException;
 import it.inaf.oats.vospace.exception.LinkFoundException;
 import it.inaf.oats.vospace.exception.NodeNotFoundException;
 import it.inaf.oats.vospace.exception.PermissionDeniedException;
 import it.inaf.oats.vospace.persistence.NodeDAO;
-import java.util.ArrayList;
 import java.util.List;
-import java.util.stream.Collectors;
 import javax.servlet.http.HttpServletRequest;
 import net.ivoa.xml.vospace.v2.Node;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.http.HttpStatus;
 import org.springframework.http.MediaType;
 import org.springframework.http.ResponseEntity;
 import org.springframework.web.bind.annotation.DeleteMapping;
-import org.springframework.web.bind.annotation.GetMapping;
 import org.springframework.web.bind.annotation.RestController;
 
 
 @RestController
 public class DeleteNodeController extends BaseNodeController  {
     
+    private static final Logger LOG = LoggerFactory.getLogger(DeleteNodeController.class);
+    
     @Autowired
     private NodeDAO nodeDAO;
 
@@ -39,6 +32,7 @@ public class DeleteNodeController extends BaseNodeController  {
     public ResponseEntity<String> deleteNode(HttpServletRequest request, User principal) {
         
         String path = getPath();
+        LOG.debug("deleteNode called for path {}", path);
         
         // Check if the node is present, 
         // if the node does not exist the service SHALL throw a HTTP 404 status code 
@@ -53,7 +47,7 @@ public class DeleteNodeController extends BaseNodeController  {
         // status code including a LinkFound fault in the entity-body if either /a 
         // or /a/b are LinkNodes.
         List<String> pathComponents = NodeUtils.subPathComponents(path);
-        if (pathComponents.size() == 0) { 
+        if (pathComponents.isEmpty()) { 
             
             // Manage root node
             throw new PermissionDeniedException("root");
diff --git a/src/main/java/it/inaf/oats/vospace/ListNodeController.java b/src/main/java/it/inaf/oats/vospace/ListNodeController.java
index e357933..4ac5fc5 100644
--- a/src/main/java/it/inaf/oats/vospace/ListNodeController.java
+++ b/src/main/java/it/inaf/oats/vospace/ListNodeController.java
@@ -10,11 +10,15 @@ import net.ivoa.xml.vospace.v2.Node;
 
 import it.inaf.oats.vospace.persistence.NodeDAO;
 import javax.servlet.http.HttpServletRequest;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.springframework.http.MediaType;
 
 @RestController
 public class ListNodeController extends BaseNodeController {
 
+    private static final Logger LOG = LoggerFactory.getLogger(ListNodeController.class);
+    
     @Autowired
     private NodeDAO nodeDAO;
 
@@ -22,6 +26,7 @@ public class ListNodeController extends BaseNodeController {
             produces = {MediaType.APPLICATION_XML_VALUE, MediaType.APPLICATION_JSON_VALUE, MediaType.TEXT_XML_VALUE})
     public ResponseEntity<Node> listNode(HttpServletRequest request) {
         String path = getPath();
+        LOG.debug("listNode called for path {}", path);
         return ResponseEntity.ok(nodeDAO.listNode(path)
                 .orElseThrow(() -> new NodeNotFoundException(path)));
     }
diff --git a/src/main/java/it/inaf/oats/vospace/UriService.java b/src/main/java/it/inaf/oats/vospace/UriService.java
index 0041ba2..f95a87e 100644
--- a/src/main/java/it/inaf/oats/vospace/UriService.java
+++ b/src/main/java/it/inaf/oats/vospace/UriService.java
@@ -4,6 +4,8 @@ import it.inaf.ia2.aa.ServletRapClient;
 import it.inaf.ia2.aa.data.User;
 import it.inaf.ia2.rap.client.call.TokenExchangeRequest;
 import it.inaf.oats.vospace.datamodel.NodeProperties;
+import static it.inaf.oats.vospace.datamodel.NodeUtils.urlEncodePath;
+import it.inaf.oats.vospace.exception.NodeNotFoundException;
 import it.inaf.oats.vospace.persistence.NodeDAO;
 import java.util.ArrayList;
 import java.util.List;
@@ -65,13 +67,11 @@ public class UriService {
 
         String relativePath = transfer.getTarget().substring("vos://".length() + authority.length());
 
-        // TODO handle node not found
-        Node node = nodeDao.listNode(relativePath).get();
+        Node node = nodeDao.listNode(relativePath).orElseThrow(() -> new NodeNotFoundException(relativePath));
 
         // TODO build the path according to node type
         //
-        // TODO add token for authenticated access
-        String endpoint = fileServiceUrl + relativePath + "?jobId=" + job.getJobId();
+        String endpoint = fileServiceUrl + urlEncodePath(relativePath) + "?jobId=" + job.getJobId();
 
         if (!"true".equals(NodeProperties.getNodePropertyByURI(node, NodeProperties.PUBLIC_READ_URI))) {
             endpoint += "&token=" + getEndpointToken(fileServiceUrl + relativePath);
diff --git a/src/main/java/it/inaf/oats/vospace/exception/InvalidURIException.java b/src/main/java/it/inaf/oats/vospace/exception/InvalidURIException.java
index c303711..085d102 100644
--- a/src/main/java/it/inaf/oats/vospace/exception/InvalidURIException.java
+++ b/src/main/java/it/inaf/oats/vospace/exception/InvalidURIException.java
@@ -7,12 +7,15 @@ import org.springframework.web.bind.annotation.ResponseStatus;
 public class InvalidURIException extends VoSpaceException {
 
     public InvalidURIException(String URI, String path) {
-        super("InvalidURI. Payload node URI: " + URI + 
-                " is not consistent with request path: " + path);
+        super("InvalidURI. Payload node URI: " + URI
+                + " is not consistent with request path: " + path);
     }
-    
-    public InvalidURIException(String URI)
-    {
-        super("InvalidURI. URI: "+URI+ " is not in a valid format");
+
+    public InvalidURIException(String URI) {
+        super("InvalidURI. URI: " + URI + " is not in a valid format");
+    }
+
+    public InvalidURIException(IllegalArgumentException ex) {
+        super("InvalidURI. " + ex.getMessage());
     }
 }
diff --git a/src/test/java/it/inaf/oats/vospace/CreateNodeControllerTest.java b/src/test/java/it/inaf/oats/vospace/CreateNodeControllerTest.java
index ab2dd44..27058b0 100644
--- a/src/test/java/it/inaf/oats/vospace/CreateNodeControllerTest.java
+++ b/src/test/java/it/inaf/oats/vospace/CreateNodeControllerTest.java
@@ -2,6 +2,7 @@ package it.inaf.oats.vospace;
 
 import it.inaf.oats.vospace.persistence.NodeDAO;
 import java.io.InputStream;
+import java.net.URI;
 import java.nio.charset.StandardCharsets;
 import net.ivoa.xml.vospace.v2.Property;
 import net.ivoa.xml.vospace.v2.UnstructuredDataNode;
@@ -23,9 +24,9 @@ import org.springframework.boot.test.mock.mockito.SpyBean;
 import net.ivoa.xml.vospace.v2.ContainerNode;
 import net.ivoa.xml.vospace.v2.LinkNode;
 import java.util.List;
-import it.inaf.ia2.aa.data.User;
 import java.util.Optional;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import org.mockito.ArgumentCaptor;
 import static org.mockito.Mockito.times;
 import org.springframework.test.context.TestPropertySource;
@@ -83,12 +84,6 @@ public class CreateNodeControllerTest {
         return parentNode;
     }
 
-    private User getUser() {
-        User user = new User();
-        user.setGroups(List.of("test1"));
-        return user;
-    }
-
     @Test
     public void testFromJsonToXml() throws Exception {
         String requestBody
@@ -299,6 +294,24 @@ public class CreateNodeControllerTest {
         assertEquals("/mydata1/anothernode", argCaptor.getAllValues().get(0));
         assertEquals("/mydata1", argCaptor.getAllValues().get(1));
     }
+    
+    @Test
+    public void testIllegalChars() throws Exception {
+
+        String requestBody = getResourceFileContent("create-unstructured-data-node.xml")
+                .replace("/mydata1", "/mydata1/?anothernode");
+
+        String message = mockMvc.perform(put(new URI("/nodes/mydata1/%3Fanothernode"))
+                .header("Authorization", "Bearer user2_token")
+                .content(requestBody)
+                .contentType(MediaType.APPLICATION_XML)
+                .accept(MediaType.APPLICATION_XML))
+                .andDo(print())
+                .andExpect(status().isBadRequest())
+                .andReturn().getResolvedException().getMessage();
+
+        assertTrue(message.contains("contains an illegal character"));
+    }
 
     private void verifyArguments() {
         verify(controller).createNode(
-- 
GitLab