Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions doc/release-notes/harden-clean-storage.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
The dataset storage cleanup call is now `PUT /api/datasets/{id}/cleanStorage`. It was a `GET`, which meant anything that follows links (browser prefetch, link previews in chat and mail clients, crawlers, or revisiting the URL from history) could trigger a deletion.

Two further changes make it safer:

- `dryrun` now defaults to `true`. Omitting it reports what would be removed instead of removing it. Pass `dryrun=false` to actually delete.
- Storage objects modified more recently than `dataverse.files.clean-storage-min-age-days` (7 by default) are never removed. An upload is written to the dataset's storage location before Dataverse registers it as a file, so without a grace period a completed upload still waiting to be saved was indistinguishable from an abandoned one and could be deleted. Raise the setting if uploads in your installation stay unregistered for longer than a week.

Scripts calling this endpoint need to switch to `PUT` and, if they relied on the old default to delete, to pass `dryrun=false` explicitly.
14 changes: 10 additions & 4 deletions doc/sphinx-guides/source/api/native-api.rst
Original file line number Diff line number Diff line change
Expand Up @@ -3462,8 +3462,14 @@ Cleanup Storage of a Dataset

This is an experimental feature and should be tested on your system before using it in production.
Also, make sure that your backups are up-to-date before using this on production servers.
It is advised to first call this method with the ``dryrun`` parameter set to ``true`` before actually deleting the files.
This will allow you to manually inspect the files that would be deleted if that parameter is set to ``false`` or is omitted (a list of the files that would be deleted is provided in the response).
This call only reports what it would remove unless ``dryrun`` is explicitly set to ``false``. Omitting the parameter is
the same as setting it to ``true``, so inspecting the reported list is always the default (a list of the files that would
be deleted is provided in the response).

Storage objects that were modified more recently than ``dataverse.files.clean-storage-min-age-days`` (7 by default) are
never removed. An upload writes its object to the dataset's storage location before Dataverse registers it as a file, so
without that grace period a completed upload waiting to be saved would look the same as an abandoned one. Raise the
setting if your workflows leave uploads unregistered for longer than a week.

If your Dataverse installation has been configured to support direct uploads, or in some other situations,
you could end up with some files in the storage of a dataset that are not linked to that dataset directly. Most commonly, this could
Expand All @@ -3479,13 +3485,13 @@ All the files stored in the Dataset storage location that are not in the file li
export PERSISTENT_ID=doi:10.5072/FK2/J8SJZB
export DRYRUN=true

curl -H "X-Dataverse-key: $API_TOKEN" -X GET "$SERVER_URL/api/datasets/:persistentId/cleanStorage?persistentId=$PERSISTENT_ID&dryrun=$DRYRUN"
curl -H "X-Dataverse-key: $API_TOKEN" -X PUT "$SERVER_URL/api/datasets/:persistentId/cleanStorage?persistentId=$PERSISTENT_ID&dryrun=$DRYRUN"

The fully expanded example above (without environment variables) looks like this:

.. code-block:: bash

curl -H "X-Dataverse-key: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" -X GET "https://demo.dataverse.org/api/datasets/:persistentId/cleanStorage?persistentId=doi:10.5072/FK2/J8SJZB&dryrun=true"
curl -H "X-Dataverse-key: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" -X PUT "https://demo.dataverse.org/api/datasets/:persistentId/cleanStorage?persistentId=doi:10.5072/FK2/J8SJZB&dryrun=true"

Adding Files To a Dataset via Other Tools
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Expand Down
5 changes: 4 additions & 1 deletion src/main/java/edu/harvard/iq/dataverse/api/Admin.java
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
import jakarta.ws.rs.core.Response;
import static edu.harvard.iq.dataverse.util.json.NullSafeJsonBuilder.jsonObjectBuilder;

import java.time.Duration;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.util.Collections;
Expand Down Expand Up @@ -2957,7 +2958,9 @@ public Response getAuditFiles(@Context ContainerRequestContext crc,
try {
Predicate<String> filter = s -> true;
StorageIO<DvObject> datasetIO = DataAccess.getStorageIO(dataset);
final List<String> result = datasetIO.cleanUp(filter, true);
// Auditing lists everything, so no minimum age applies here. The
// listing is not used; the call is kept for its existence checks.
datasetIO.cleanUp(filter, Duration.ZERO, true);
// add files that are in dataset files but not in cleanup result or DataFiles with missing FileMetadata
dataset.getFiles().forEach(df -> {
try {
Expand Down
19 changes: 14 additions & 5 deletions src/main/java/edu/harvard/iq/dataverse/api/Datasets.java
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@
import java.sql.Timestamp;
import java.text.MessageFormat;
import java.text.SimpleDateFormat;
import java.time.Duration;
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.ZoneId;
Expand Down Expand Up @@ -1997,7 +1998,7 @@

// dateAvailable is within limits
if (minRetentionDateTime != null){
if (dateUnavailable.isBefore(minRetentionDateTime.minusDays(1))){

Check warning on line 2001 in src/main/java/edu/harvard/iq/dataverse/api/Datasets.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Merge this if statement with the enclosing one.

See more on https://sonarcloud.io/project/issues?id=IQSS_dataverse&issues=AaCL3HFZFvIBsNCIqGSs&open=AaCL3HFZFvIBsNCIqGSs&pullRequest=12690
return error(Status.BAD_REQUEST, "Date unavailable can not be earlier than MinRetentionDurationInMonths: "+minRetentionDurationInMonths + " from now");
}
}
Expand Down Expand Up @@ -3410,11 +3411,14 @@
* @param idSupplied
* @return
*/
@GET
@PUT
@AuthRequired
@Path("{id}/cleanStorage")
@Operation(summary = "Cleans dataset storage",
description = "Finds and optionally deletes storage objects that are no longer referenced by files in a dataset.")
description = "Finds and optionally deletes storage objects that are no longer referenced by files in a dataset. "
+ "Reports without deleting unless dryrun is explicitly set to false. Objects modified more recently than "
+ "dataverse.files.clean-storage-min-age-days are never removed, so that an upload which has completed but "
+ "has not been registered yet is not mistaken for an abandoned one.")
public Response cleanStorage(@Context ContainerRequestContext crc, @Parameter(description = "Resource id or persistent identifier.") @PathParam("id") String idSupplied, @Parameter(description = "Whether to validate the request without applying changes.") @QueryParam("dryrun") Boolean dryrun) {
// get user and dataset
User authUser = getRequestUser(crc);
Expand All @@ -3428,10 +3432,11 @@

// check permissions
if (!permissionSvc.permissionsFor(req, dataset).contains(Permission.EditDataset)) {
return error(Response.Status.INTERNAL_SERVER_ERROR, "Access denied!");
return error(Response.Status.FORBIDDEN, "Access denied!");
}

boolean doDryRun = dryrun != null && dryrun.booleanValue();
// Reporting is the default: deleting requires asking for it explicitly.
boolean doDryRun = dryrun == null || dryrun.booleanValue();

// check if no legacy files are present
Set<String> datasetFilenames = getDatasetFilenames(dataset);
Expand All @@ -3443,7 +3448,7 @@
List<String> deleted;
try {
StorageIO<DvObject> datasetIO = DataAccess.getStorageIO(dataset);
deleted = datasetIO.cleanUp(filter, doDryRun);
deleted = datasetIO.cleanUp(filter, getCleanStorageMinimumAge(), doDryRun);
} catch (IOException ex) {
logger.log(Level.SEVERE, null, ex);
return error(Response.Status.INTERNAL_SERVER_ERROR, "IOException! Serious Error! See administrator!");
Expand Down Expand Up @@ -3499,6 +3504,10 @@
}, getRequestUser(crc));
}

private static Duration getCleanStorageMinimumAge() {
return Duration.ofDays(JvmSettings.CLEAN_STORAGE_MIN_AGE_DAYS.lookup(Integer.class));
}

private static Set<String> getDatasetFilenames(Dataset dataset) {
Set<String> files = new HashSet<>();
for (DataFile dataFile: dataset.getFiles()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import java.security.KeyManagementException;
import java.security.KeyStoreException;
import java.security.NoSuchAlgorithmException;
import java.time.Duration;
import java.util.List;
import java.util.function.Predicate;
import java.util.logging.Logger;
Expand Down Expand Up @@ -213,8 +214,8 @@ public boolean exists() {
}

@Override
public List<String> cleanUp(Predicate<String> filter, boolean dryRun) throws IOException {
return baseStore.cleanUp(filter, dryRun);
public List<String> cleanUp(Predicate<String> filter, Duration minimumAge, boolean dryRun) throws IOException {
return baseStore.cleanUp(filter, minimumAge, dryRun);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,13 @@
import java.nio.file.InvalidPathException;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.time.Duration;
import java.time.Instant;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.function.Predicate;
import java.util.logging.Logger;
import java.util.stream.Collectors;

// Dataverse imports:
import edu.harvard.iq.dataverse.DataFile;
Expand Down Expand Up @@ -676,7 +679,7 @@ protected static boolean isValidIdentifier(String driverId, String storageId) {
return true;
}

private List<String> listAllFiles() throws IOException {
private Map<String, Instant> listAllFiles() throws IOException {
Dataset dataset = this.getDataset();
if (dataset == null) {
throw new IOException("This FileAccessIO object hasn't been properly initialized.");
Expand All @@ -689,10 +692,17 @@ private List<String> listAllFiles() throws IOException {

DirectoryStream<Path> dirStream = Files.newDirectoryStream(Paths.get(this.getFilesRootDirectory(), datasetDirectoryPath.toString()));

List<String> res = new ArrayList<>();
Map<String, Instant> res = new HashMap<>();
if (dirStream != null) {
for (Path filePath : dirStream) {
res.add(filePath.getFileName().toString());
Instant lastModified;
try {
lastModified = Files.getLastModifiedTime(filePath).toInstant();
} catch (IOException ex) {
// Unknown age is treated as too recent to remove.
lastModified = null;
}
res.put(filePath.getFileName().toString(), lastModified);
}
dirStream.close();
}
Expand All @@ -716,8 +726,8 @@ private void deleteFile(String fileName) throws IOException {
}

@Override
public List<String> cleanUp(Predicate<String> filter, boolean dryRun) throws IOException {
List<String> toDelete = this.listAllFiles().stream().filter(filter).collect(Collectors.toList());
public List<String> cleanUp(Predicate<String> filter, Duration minimumAge, boolean dryRun) throws IOException {
List<String> toDelete = selectForCleanUp(this.listAllFiles(), filter, minimumAge);
if (dryRun) {
return toDelete;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import java.nio.channels.Channels;
import java.nio.channels.WritableByteChannel;
import java.nio.file.Path;
import java.time.Duration;
import java.util.List;
import java.util.function.Predicate;
import java.util.logging.Logger;
Expand Down Expand Up @@ -161,7 +162,7 @@ public void revertBackupAsAux(String auxItemTag) throws IOException {
}

@Override
public List<String> cleanUp(Predicate<String> filter, boolean dryRun) throws IOException {
public List<String> cleanUp(Predicate<String> filter, Duration minimumAge, boolean dryRun) throws IOException {
throw new UnsupportedDataAccessOperationException("InputStreamIO: tthis method is not supported in this DataAccess driver.");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@
import java.nio.file.Paths;
import java.time.Duration;
import java.time.Instant;
import java.util.HashMap;
import java.util.Map;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Date;
Expand Down Expand Up @@ -1517,7 +1519,7 @@ protected static boolean isValidIdentifier(String driverId, String storageId) {
return true;
}

private List<String> listAllFiles() throws IOException {
private Map<String, Instant> listAllFiles() throws IOException {
if (!this.canWrite()) {
open();
}
Expand All @@ -1527,7 +1529,7 @@ private List<String> listAllFiles() throws IOException {
}
String prefix = dataset.getAuthorityForFileStorage() + "/" + dataset.getIdentifierForFileStorage() + "/";

List<String> ret = new ArrayList<>();
Map<String, Instant> ret = new HashMap<>();
ListObjectsV2Request listObjectsReqManual = ListObjectsV2Request.builder().bucket(bucketName).prefix(prefix)
.build();

Expand Down Expand Up @@ -1571,7 +1573,7 @@ private List<String> listAllFiles() throws IOException {

for (S3Object item : storedFilesSummary) {
String fileName = item.key().substring(prefix.length());
ret.add(fileName);
ret.put(fileName, item.lastModified());
}
return ret;
}
Expand Down Expand Up @@ -1623,8 +1625,8 @@ public void closeInputStream() {
}

@Override
public List<String> cleanUp(Predicate<String> filter, boolean dryRun) throws IOException {
List<String> toDelete = this.listAllFiles().stream().filter(filter).collect(Collectors.toList());
public List<String> cleanUp(Predicate<String> filter, Duration minimumAge, boolean dryRun) throws IOException {
List<String> toDelete = selectForCleanUp(this.listAllFiles(), filter, minimumAge);
if (dryRun) {
return toDelete;
}
Expand Down
39 changes: 38 additions & 1 deletion src/main/java/edu/harvard/iq/dataverse/dataaccess/StorageIO.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
import java.nio.channels.WritableByteChannel;
import java.nio.file.Path;
import java.util.HashMap;
import java.time.Duration;
import java.time.Instant;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -641,7 +643,42 @@ protected static boolean usesStandardNamePattern(String identifier) {
return m.find();
}

public abstract List<String> cleanUp(Predicate<String> filter, boolean dryRun) throws IOException;
/**
* Deletes stored objects that {@code filter} selects and that have not been modified
* for at least {@code minimumAge}.
*
* @param filter selects objects by name, typically those no longer referenced by the dataset
* @param minimumAge how long an object must have been untouched before it can be removed
* @param dryRun when true, report what would be removed without removing anything
*/
public abstract List<String> cleanUp(Predicate<String> filter, Duration minimumAge, boolean dryRun) throws IOException;

/**
* The stored objects that {@code filter} selects and that are old enough to
* remove. Shared by the storage backends so the selection rule has one
* definition.
*
* @param stored object names mapped to their last modification time
*/
protected static List<String> selectForCleanUp(Map<String, Instant> stored, Predicate<String> filter,
Duration minimumAge) {
return stored.entrySet().stream()
.filter(e -> filter.test(e.getKey()) && isOlderThan(e.getValue(), minimumAge))
.map(Map.Entry::getKey)
.toList();
}

/**
* Whether an object last modified at {@code lastModified} is old enough to remove.
* An unknown timestamp is treated as too recent, so that a storage backend which
* cannot report one never causes a deletion.
*/
protected static boolean isOlderThan(Instant lastModified, Duration minimumAge) {
if (lastModified == null) {
return false;
}
return lastModified.isBefore(Instant.now().minus(minimumAge));
}

/**
* A storage-type-specific mechanism for retrieving the size of a file. Intended
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,15 @@
import java.security.SignatureException;
import java.util.ArrayList;
import java.util.Collection;
import java.time.Duration;
import java.time.Instant;
import java.util.HashMap;
import java.util.Map;
import java.util.Formatter;
import java.util.List;
import java.util.Properties;
import java.util.function.Predicate;
import java.util.logging.Logger;
import java.util.stream.Collectors;

import javax.crypto.Mac;
import javax.crypto.spec.SecretKeySpec;
Expand Down Expand Up @@ -912,7 +915,7 @@ public static String calculateRFC2104HMAC(String data, String key)
return toHexString(mac.doFinal(data.getBytes()));
}

private List<String> listAllFiles() throws IOException {
private Map<String, Instant> listAllFiles() throws IOException {
if (!this.canWrite()) {
open(DataAccessOption.WRITE_ACCESS);
}
Expand All @@ -924,12 +927,19 @@ private List<String> listAllFiles() throws IOException {

Collection<StoredObject> items;
String lastItemName = null;
List<String> ret = new ArrayList<>();
Map<String, Instant> ret = new HashMap<>();

while ((items = this.swiftContainer.list(prefix, lastItemName, LIST_PAGE_LIMIT)) != null && items.size() > 0) {
for (StoredObject item : items) {
lastItemName = item.getName().substring(prefix.length());
ret.add(lastItemName);
Instant lastModified;
try {
lastModified = item.getLastModifiedAsDate().toInstant();
} catch (RuntimeException ex) {
// Unknown age is treated as too recent to remove.
lastModified = null;
}
ret.put(lastItemName, lastModified);
}
}

Expand All @@ -956,8 +966,8 @@ private void deleteFile(String fileName) throws IOException {
}

@Override
public List<String> cleanUp(Predicate<String> filter, boolean dryRun) throws IOException {
List<String> toDelete = this.listAllFiles().stream().filter(filter).collect(Collectors.toList());
public List<String> cleanUp(Predicate<String> filter, Duration minimumAge, boolean dryRun) throws IOException {
List<String> toDelete = selectForCleanUp(this.listAllFiles(), filter, minimumAge);
if (dryRun) {
return toDelete;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ public enum JvmSettings {
FEATURED_ITEMS_IMAGE_UPLOADS_DIRECTORY(SCOPE_FEATURED_ITEMS, "image-uploads"),
HIDE_SCHEMA_DOT_ORG_DOWNLOAD_URLS(SCOPE_FILES, "hide-schema-dot-org-download-urls"),
DEFAULT_DATASET_FILE_COUNT_LIMIT(SCOPE_FILES, "default-dataset-file-count-limit"),
CLEAN_STORAGE_MIN_AGE_DAYS(SCOPE_FILES, "clean-storage-min-age-days"),

//STORAGE DRIVER SETTINGS
SCOPE_DRIVER(SCOPE_FILES),
Expand Down
3 changes: 3 additions & 0 deletions src/main/resources/META-INF/microprofile-config.properties
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ dataverse.files.directory=${STORAGE_DIR:/tmp/dataverse}
dataverse.files.uploads=${STORAGE_DIR:${com.sun.aas.instanceRoot}}/uploads
dataverse.files.docroot=${STORAGE_DIR:${com.sun.aas.instanceRoot}}/docroot
dataverse.files.globus-cache-maxage=5
# Storage objects younger than this are left alone by cleanStorage, so that an upload
# that has finished but has not been registered yet is not mistaken for junk.
dataverse.files.clean-storage-min-age-days=7
dataverse.files.featured-items.image-maxsize=1000000
dataverse.files.featured-items.image-uploads=featuredItems

Expand Down
Loading
Loading