GDCC/Update storageIO processing to match RemoteOverlay

src/main/java/edu/harvard/iq/dataverse/dataaccess/DataAccess.java

@@ -331,14 +331,23 @@ public static boolean uploadToDatasetAllowed(Dataset d, String storageIdentifier
         String driverId = DataAccess.getStorageDriverFromIdentifier(storageIdentifier);
         String effectiveDriverId = d.getEffectiveStorageDriverId();
         if(!effectiveDriverId.equals(driverId)) {
+            //Not allowed unless this is a remote store and you're uploading to the basestore
             if(getDriverType(driverId).equals(REMOTE)) {
                 String baseDriverId = RemoteOverlayAccessIO.getBaseStoreIdFor(driverId);
                 if(!effectiveDriverId.equals(baseDriverId)) {
+                    //Not allowed - wrong base driver
                     allowed = false;
+                } else {
+                    //Only allowed if baseStore allows it
+                    allowed = StorageIO.isDirectUploadEnabled(baseDriverId);
                 }
             } else {
+                //Not allowed - wrong main driver
                 allowed=false;
             }
+        } else {
+            //Only allowed if main store allows it
+            allowed = StorageIO.isDirectUploadEnabled(driverId);
         }
         return allowed;
     }

src/main/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIO.java

@@ -29,10 +29,13 @@
 import java.io.FileOutputStream;
 // NIO imports: 
 import java.nio.file.Files;
+import java.nio.file.InvalidPathException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.List;
 import java.util.logging.Logger;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 // Dataverse imports:
 import edu.harvard.iq.dataverse.DataFile;
@@ -303,7 +306,7 @@ public Path getAuxObjectAsPath(String auxItemTag) throws IOException {
             throw new IOException("Null or invalid Auxiliary Object Tag.");
         }
         if(isDirectAccess()) {
-            //Overlay case
+            //includes overlay case
             return Paths.get(physicalPath.toString() + "." + auxItemTag);
         }
         String datasetDirectory = getDatasetDirectory();
@@ -427,8 +430,8 @@ public void deleteAllAuxObjects() throws IOException {
         }
 
     }
-    
-    
+
+
     @Override
     public String getStorageLocation() {
         // For a local file, the "storage location" is a complete, absolute
@@ -656,4 +659,28 @@ private String stripDriverId(String storageIdentifier) {
         }
         return storageIdentifier;
     }
+
+    //Confirm inputs are of the form of a relative file path that doesn't contain . or ..
+    protected static boolean isValidIdentifier(String driverId, String storageId) {
+        String pathString = storageId.substring(storageId.lastIndexOf("//") + 2);
+        String basePath = "/tmp/";
+        try {
+            String rawPathString = basePath + pathString;
+            Path normalized = Paths.get(rawPathString).normalize();
+            if(!rawPathString.equals(normalized.toString())) {
+                logger.warning("Non-normalized path in submitted identifier " + storageId);
+                return false;
+            }
+            logger.fine(normalized.getFileName().toString());
+            if (!usesStandardNamePattern(normalized.getFileName().toString())) {
+                logger.warning("Unacceptable file name in submitted identifier: " + storageId);
+                return false;
+            }
+
+        } catch (InvalidPathException ipe) {
+            logger.warning("Invalid Path in submitted identifier " + storageId);
+            return false;
+        }
+        return true;
+    }
 }

src/main/java/edu/harvard/iq/dataverse/dataaccess/RemoteOverlayAccessIO.java

@@ -630,4 +630,5 @@ protected static boolean isValidIdentifier(String driverId, String storageId) {
     public static String getBaseStoreIdFor(String driverId) {
         return System.getProperty("dataverse.files." + driverId + ".base-store");
     }
+
 }

src/main/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIO.java

@@ -937,9 +937,9 @@ public String generateTemporaryDownloadUrl(String auxiliaryTag, String auxiliary
                     // them for some servers, we check whether the protocol is in the url and then
                     // normalizing to use the part without the protocol
                     String endpointServer = endpoint;
-                    int protocolEnd = endpoint.indexOf("://");
+                    int protocolEnd = endpoint.indexOf(DataAccess.SEPARATOR);
                     if (protocolEnd >=0 ) {
-                        endpointServer = endpoint.substring(protocolEnd + 3);
+                        endpointServer = endpoint.substring(protocolEnd + DataAccess.SEPARATOR.length());
                     }
                     logger.fine("Endpoint: " + endpointServer);
                     // We're then replacing 
@@ -998,9 +998,9 @@ private String generateTemporaryS3UploadUrl(String key, Date expiration) throws
                 // them for some servers, we check whether the protocol is in the url and then
                 // normalizing to use the part without the protocol
                 String endpointServer = endpoint;
-                int protocolEnd = endpoint.indexOf("://");
+                int protocolEnd = endpoint.indexOf(DataAccess.SEPARATOR);
                 if (protocolEnd >=0 ) {
-                    endpointServer = endpoint.substring(protocolEnd + 3);
+                    endpointServer = endpoint.substring(protocolEnd + DataAccess.SEPARATOR.length());
                 }
                 logger.fine("Endpoint: " + endpointServer);
                 // We're then replacing 
@@ -1274,5 +1274,37 @@ public boolean isMainDriver() {
     public void setMainDriver(boolean mainDriver) {
         this.mainDriver = mainDriver;
     }
+
+    public static String getDriverPrefix(String driverId) {
+        return driverId+ DataAccess.SEPARATOR + getBucketName(driverId) + ":";
+    }
+
+    //Confirm inputs are of the form s3://demo-dataverse-bucket:176e28068b0-1c3f80357c42
+    protected static boolean isValidIdentifier(String driverId, String storageId) {
+        String storageBucketAndId = storageId.substring(storageId.lastIndexOf("//") + 2);
+        String bucketName = getBucketName(driverId);
+        if(bucketName==null) {
+            logger.warning("No bucket defined for " + driverId);
+            return false;
+        }
+        int index = storageBucketAndId.lastIndexOf(":");
+        if(index<=0) {
+            logger.warning("No bucket defined in submitted identifier: " + storageId);
+            return false;
+        }
+        String idBucket = storageBucketAndId.substring(0, index);
+        String id = storageBucketAndId.substring(index+1);
+        logger.fine(id);
+        if(!bucketName.equals(idBucket)) {
+            logger.warning("Incorrect bucket in submitted identifier: " + storageId);
+            return false;
+        }
+        if (!usesStandardNamePattern(id)) {
+            logger.warning("Unacceptable identifier pattern in submitted identifier: " + storageId);
+            return false;
+        }
+        return true;
+    }
+
 
 }

src/main/java/edu/harvard/iq/dataverse/dataaccess/StorageIO.java

@@ -594,10 +594,14 @@ public static String getDriverPrefix(String driverId) {
         return driverId+ DataAccess.SEPARATOR;
     }
 
+    public static boolean isDirectUploadEnabled(String driverId) {
+        return Boolean.getBoolean(System.getProperty("dataverse.files." + driverId + ".download-redirect", "false"));
+    }
+
     //Check that storageIdentifier is consistent with store's config
     //False will prevent direct uploads
     protected static boolean isValidIdentifier(String driverId, String storageId) {
-        return true;
+        return false;
     }
 
     //Utility to verify the standard UUID pattern for stored files.
@@ -607,4 +611,5 @@ protected static boolean usesStandardNamePattern(String identifier) {
         Matcher m = r.matcher(identifier);
         return m.find();
     }
+
 }

src/main/resources/db/migration/V5.11.1.6__storageconstraint.sql

@@ -0,0 +1,10 @@
+DO $$
+BEGIN
+
+  BEGIN
+    ALTER TABLE dvobject ADD CONSTRAINT chk_dvobject_storageidentifier check (strpos(storageidentifier,'..') = 0);
+  EXCEPTION
+    WHEN duplicate_object THEN RAISE NOTICE 'Table constraint chk_dvobject_storageidentifier already exists';
+  END;
+
+END $$;

src/test/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIOTest.java

@@ -9,6 +9,8 @@
 import edu.harvard.iq.dataverse.Dataset;
 import edu.harvard.iq.dataverse.Dataverse;
 import edu.harvard.iq.dataverse.mocks.MocksFactory;
+import edu.harvard.iq.dataverse.util.FileUtil;
+
 import java.io.BufferedReader;
 import java.io.BufferedWriter;
 import java.io.ByteArrayInputStream;
@@ -24,6 +26,9 @@
 import org.junit.After;
 import org.junit.Test;
 import static org.junit.Assert.*;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 import org.junit.Before;
 
 /**
@@ -285,4 +290,31 @@ public void testGetAuxFileAsInputStream() throws IOException {
         }
         assertEquals("This is a test string\n", sb.toString());
     }
+
+    @Test
+    public void testFileIdentifierFormats() throws IOException {
+        System.setProperty("dataverse.files.filetest.type", "file");
+        System.setProperty("dataverse.files.filetest.label", "Ftest");
+        System.setProperty("dataverse.files.filetest.directory", "/tmp/mydata");
+
+        FileUtil.generateStorageIdentifier();
+        assertTrue(DataAccess.isValidDirectStorageIdentifier("filetest://" + FileUtil.generateStorageIdentifier()));
+        //The tests here don't use a valid identifier string
+        assertFalse(DataAccess.isValidDirectStorageIdentifier(dataFile.getStorageIdentifier()));
+        //bad store id
+        String defaultType = System.getProperty("dataverse.files.file.type");
+        //Assure file isn't a defined store before test and reset afterwards if it was
+        System.clearProperty("dataverse.files.file.type");
+        assertFalse(DataAccess.isValidDirectStorageIdentifier("file://" + FileUtil.generateStorageIdentifier()));
+        if(defaultType!=null) {
+            System.out.println("dataverse.files.file.type reset to " + defaultType);
+            System.setProperty("dataverse.files.file.type", defaultType);
+        }
+        //breakout
+        assertFalse(DataAccess.isValidDirectStorageIdentifier("filetest://../" + FileUtil.generateStorageIdentifier()));
+
+        System.clearProperty("dataverse.files.filetest.type");
+        System.clearProperty("dataverse.files.filetest.label");
+        System.clearProperty("dataverse.files.filetest.directory");
+    }
 }

src/test/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIOTest.java

@@ -9,6 +9,9 @@
 import edu.harvard.iq.dataverse.Dataset;
 import edu.harvard.iq.dataverse.api.UtilIT;
 import edu.harvard.iq.dataverse.mocks.MocksFactory;
+import edu.harvard.iq.dataverse.util.FileUtil;
+
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
@@ -42,11 +45,21 @@ public void setup() throws IOException {
         dataSet = MocksFactory.makeDataset();
         dataFile.setOwner(dataSet);
         dataFileId = UtilIT.getRandomIdentifier();
-        dataFile.setStorageIdentifier("s3://bucket:"+dataFileId);
-        dataSetAccess = new S3AccessIO<>(dataSet, null, s3client, "s3");
-        dataFileAccess = new S3AccessIO<>(dataFile, null, s3client, "s3");
+        System.setProperty("dataverse.files.s3test.type", "s3");
+        System.setProperty("dataverse.files.s3test.label", "S3test");
+        System.setProperty("dataverse.files.s3test.bucket-name", "thebucket");
+
+        dataFile.setStorageIdentifier("s3test://thebucket:"+dataFileId);
+        dataSetAccess = new S3AccessIO<>(dataSet, null, s3client, "s3test");
+        dataFileAccess = new S3AccessIO<>(dataFile, null, s3client, "s3test");
     }
 
+    @AfterEach
+    public void tearDown() {
+        System.clearProperty("dataverse.files.s3test.type");
+        System.clearProperty("dataverse.files.s3test.label");
+        System.clearProperty("dataverse.files.s3test.bucket-name");
+    }
     /*
     createTempFile
     getStorageLocation
@@ -99,25 +112,36 @@ void keyNullstorageIdInvalid_getMainFileKey() throws IOException {
     @Test
     void default_getUrlExpirationMinutes() {
         // given
-        System.clearProperty("dataverse.files.s3.url-expiration-minutes");
+        System.clearProperty("dataverse.files.s3test.url-expiration-minutes");
         // when & then
         assertEquals(60, dataFileAccess.getUrlExpirationMinutes());
     }
 
     @Test
     void validSetting_getUrlExpirationMinutes() {
         // given
-        System.setProperty("dataverse.files.s3.url-expiration-minutes", "120");
+        System.setProperty("dataverse.files.s3test.url-expiration-minutes", "120");
         // when & then
         assertEquals(120, dataFileAccess.getUrlExpirationMinutes());
     }
 
     @Test
     void invalidSetting_getUrlExpirationMinutes() {
         // given
-        System.setProperty("dataverse.files.s3.url-expiration-minutes", "NaN");
+        System.setProperty("dataverse.files.s3test.url-expiration-minutes", "NaN");
         // when & then
         assertEquals(60, dataFileAccess.getUrlExpirationMinutes());
     }
 
+    @Test
+    void testS3IdentifierFormats() throws IOException {
+        assertTrue(DataAccess.isValidDirectStorageIdentifier("s3test://thebucket:" + FileUtil.generateStorageIdentifier()));
+        //The tests here don't use a valid identifier string
+        assertFalse(DataAccess.isValidDirectStorageIdentifier(dataFile.getStorageIdentifier()));
+        //bad store id
+        assertFalse(DataAccess.isValidDirectStorageIdentifier("s3://thebucket:" + FileUtil.generateStorageIdentifier()));
+        //bad bucket
+        assertFalse(DataAccess.isValidDirectStorageIdentifier("s3test://bucket:" + FileUtil.generateStorageIdentifier()));
+    }
+
 }